[Bug-Candidate]: Invalid signature returned by `_extract_function_relations()`

[Boyan-MILANOV]

Describe the issue:

Trying to get function relations on the contract of the Fallback ethernaut challenge returns a result that contains an invalid function signature: ()

{
  "constructor()": {
    "impacts": [
      "contribute()",
      "getContribution()",
      "withdraw()",
      "()"
    ],
    "is_impacted_by": []
  },
  ...

I think slither returns () instead of receive() for the receive function prototype

Code example to reproduce the issue:

contract Fallback {

  using SafeMath for uint256;
  mapping(address => uint) public contributions;
  address payable public owner;

  constructor() public {
    owner = msg.sender;
    contributions[msg.sender] = 1000 * (1 ether);
  }

  modifier onlyOwner {
        require(
            msg.sender == owner,
            "caller is not the owner"
        );
        _;
    }

  function contribute() public payable {
    require(msg.value < 0.001 ether);
    contributions[msg.sender] += msg.value;
    if(contributions[msg.sender] > contributions[owner]) {
      owner = msg.sender;
    }
  }

  function getContribution() public view returns (uint) {
    return contributions[msg.sender];
  }

  function withdraw() public onlyOwner {
    owner.transfer(address(this).balance);
  }

  receive() external payable {
    require(msg.value > 0 && contributions[msg.sender] > 0);
    owner = msg.sender;
  }
}

Version:

0.8.3

Relevant log output:

No response

[plotchy]

This is used in the echidna printer and relies on the _get_name() helper function.

https://github.com/crytic/slither/blob/db703d82f4876e30f1ca10fb78d85b4dd4c31ec1/slither/printers/guidance/echidna.py#L35-L41

def _get_name(f: Union[Function, Variable]) -> str:
    # Return the name of the function or variable
    if isinstance(f, Function):
        if f.is_fallback or f.is_receive:
            return "()"
        return f.solidity_signature
    return f.function_name

Can be adjusted to:

def _get_name(f: Union[Function, Variable]) -> str:
    # Return the name of the function or variable
    if isinstance(f, Function):
        if f.is_fallback:
            return "fallback()"
        elif f.is_receive:
            return "receive()"
    return f.solidity_signature

Now provides output:

"constructor()": {
                "impacts": [
                    "contribute()",
                    "getContribution()",
                    "withdraw()",
                    "receive()"
                ],
                "is_impacted_by": []
            },
...
"receive()": {
                "impacts": [
                    "contribute()",
                    "withdraw()"
                ],
                "is_impacted_by": [
                    "constructor()",
                    "contribute()"
                ]
            }

[montyly]

Hi @plotchy, that's a great point :)

We actually already changed _get_name to use. solidity_signature instead of function_name (with https://github.com/crytic/slither/pull/1323): https://github.com/crytic/slither/blob/5a6b6309c5bb1e550e47e01c4a400180be1a986b/slither/printers/guidance/echidna.py#L35-L40

However I am a bit unsure about returning fallback() or receive() here. A contract can have a function named fallback (same for receive):

contract C{
    function fallback() public{
    }       
    fallback() external{
    }       
}

So it would create collision/confusion.

However maybe we could use a special placeholder for the impact/is_impacted_by list, to indicate if the destination is the receiver/fallback.

[plotchy]

Ah, @montyly youre totally right. I like the idea of using placeholders. I briefly tested with *receive() and *fallback(), but I seem to be getting collision issues within the Slither detector.

Assuming the list is getting deduplicated on names or something.

contract Fallback {
  function fallback() public payable {}
  function receive() public payable {}

  receive() external payable {}
  fallback() external payable {}
}

{
    "payable": {
        "Fallback": [
            "*fallback()",  
            "*receive()"   < -- "fallback()" and "receive()" Not recognized here
        ]
    },
    "functions_relations": {
        "Fallback": {
            "*fallback()": {
                "impacts": [],
                "is_impacted_by": []
            },
            "*receive()": {
                "impacts": [],
                "is_impacted_by": []
            }
        }
    },
    "constructors": {},
    "have_external_calls": {},
    "call_a_parameter": {},
    "use_balance": {},
    "solc_versions": [
        "0.8.13"
    ],
    "with_fallback": [ <-- Are recognized here
        "Fallback"
    ],
    "with_receive": [  <-- Are recognized here
        "Fallback"
    ]
}

Even if the functions impact each other and should all be in the impact_list, only the *receive() or *fallback() were listed.

[plotchy]

@0xalpharush Hmm after looking at that line it seems ambiguous how functions_entry_points is designed with regards to fallback. IIRC Python has higher precedence on and than or. I think the below is the way it runs: if (f.visibility in ["public", "external"] and not f.is_shadowed) or f.is_fallback . Seems that it should return fallback and receive as they are entry points.

Below is a minimal way to test impacted_list. Commenting out the fallback() external payable or receive() external payable will adjust how the impacted lists react.

contract Fallback {
    mapping(address => uint) public contributions;
    address payable public owner;

  constructor() public {
    owner = payable(msg.sender);
    contributions[msg.sender] = 1000 * (1 ether);
  }

  function fallback() public payable {
    contributions[msg.sender] += msg.value;
  }

  function receive() public payable {
    contributions[msg.sender] += msg.value;
  }

  receive() external payable {
    contributions[msg.sender] += msg.value;
  }

  fallback() external payable {
    contributions[msg.sender] += msg.value;
  }
}

[0xalpharush]

You're right that it's not the source of the issue. At a glance, I suspect the canonical_name might be identical and messing up the contract parsing in solc_parsing/.