unforgiven - function _contractFallback() is always revert because IERC677Receiver has wrong function signature for onTokenTransfer()

[sherlock-admin]

unforgiven

medium

function _contractFallback() is always revert because IERC677Receiver has wrong function signature for onTokenTransfer()

Summary

because IERC677Receiver has wrong function signature for onTokenTransfer() so the transferAndCall() will always revert if it calls onTokenTransfer() of the target contract(which correctly implement the ERC)

Vulnerability Detail

This is transferAndCall() code:

    function transferAndCall(address _to, uint256 _value, bytes calldata _data) public returns (bool success) {
        super.transfer(_to, _value);
        emit Transfer(msg.sender, _to, _value, _data);
        if (Address.isContract(_to)) {
            _contractFallback(_to, _value, _data);
        }
        return true;
    }

    // PRIVATE
    function _contractFallback(address _to, uint256 _value, bytes calldata _data) private {
        IERC677Receiver receiver = IERC677Receiver(_to);
        receiver.onTokenTransfer(msg.sender, _value, _data);
    }

as you can see it uses IERC677Receiver to call onTokenTransfer() function of the to address. This is IERC677Receiver:

interface IERC677Receiver {
    function onTokenTransfer(address _sender, uint256 _value, bytes calldata _data) external;
}

the issue is that function signature of the onTokenTransfer() is not according to the ERC677 and it doesn't define the return value of the function and if a target contract implements ERC677 correctly then it's gonna return the bool variable and because function signature is wrong in ERC677Token so solidity would revert that call and the whole transaction would revert. as result ERC677Token won't gonna work according to the EIP.

Impact

token ERC677Token would always revert in transferAndCall() function if the to is contract and that contract implements EIP correctly.

Code Snippet

https://github.com/sherlock-audit/2023-12-truflation/blob/37ddbb69e0c7fb6510f1ec99162fd9172ec44733/truflation-contracts/src/token/ERC677Token.sol#L26-L29
https://github.com/sherlock-audit/2023-12-truflation/blob/37ddbb69e0c7fb6510f1ec99162fd9172ec44733/truflation-contracts/src/interfaces/IERC677Receiver.sol#L4-L6

Tool used

Manual Review

Recommendation

change IERC677Receiver to be like this:

interface IERC677Receiver {
    function onTokenTransfer(address _sender, uint256 _value, bytes calldata _data) external returns(bool);
}

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Shaheen commented:

Not sure, maybe a valid medium

[IllIllI000]

This is invalid. The function signature that the actual 667 code uses does not have a bool, and the delployed link contracts also do not have bools on both Eth and Base mainnets

[ryuheimat]

@IllIllI000 ethereum/EIPs#677
The standard EIP677 requires bool return value i think.

[ryuheimat]

@IllIllI000 https://github.com/truflation/truflation-contracts/pull/8
I've updated in this PR anyways, and let's discuss again if we merge this or not.

[IllIllI000]

You've linked to an open issue in the EIP repo, from which a standard is usually created. In this case, the filer closed the issue without it becoming a standard, so whatever its contents at that point were, aren't really relevant.

[ryuheimat]

@IllIllI000 I agree with this, then we can mark this issue as invalid,

[nevillehuang]

@IllIllI000 Could I understand what is the purpose of the fallback function? From my understanding it is to ensure tokens are correctly received within a smart contract correct? So even if a boolean return value is not implemented, can it be assumed that the callback function would correct revert based on its logic if tokens are not successfully received?

[IllIllI000]

The purpose of EIP677's transferAndCall() is essentially to be a multicall whose task is to perform a normal transfer(), followed by an arbitrary function call on the recipient. The assumption of the EIP is that the function called after the transfer will require tokens, and any user that calls it without those tokens will get a failure. The token itself reverts on failure to transfer, so what happens afterwards doesn't seem relevant to this function, given that the expected behavior depends on the function being called by the callback afterwards, is not known ahead of time.