executing instruction outside code can lead to failing transfer

[code423n4]

Handle

Omik

Vulnerability details

Impact

in the https://github.com/code-423n4/2021-08-notional/blob/main/contracts/internal/balances/TokenHandler.sol#L221-260, is handling transfer and transferfrom, and checking the return value of the transfer and transferfrom, but the checking is happening outside the code, therefore if the transfer successfull it will still return false

Proof of Concept

1. deploy this contract in the remix pragma solidity ^0.8.0;

contract simpleerc{

function transferFrom()external returns(bool){
    return true;
}

}

2. deploy this test contract in the remix pragma solidity >0.7.0;

interface simpleerc{ function transferFrom()external returns(bool); }

contract test{

function doTransferOut(address token) public returns (bool) {
    simpleerc token = simpleerc(token);
    token.transferFrom();
    //checkreturndata();

    bool success;
    bool returndata;
    assembly {
        switch returndatasize()
            case 0 {                      // This is a non-standard ERC-20
                success := not(0)          // set success to true
            }
            case 32 {                     // This is a complaint ERC-20
                returndatacopy(0, 0, 32)
                success := mload(0)        // Set `success = returndata` of external call
            }
            default {                     // This is an excessively non-compliant ERC-20, revert.
                revert(0, 0)
            }
    }
    return success;

}

function doTransferOutwithcheck(address token) public returns (bool) {
    simpleerc token = simpleerc(token);
    token.transferFrom();
    checkreturndata();

}

function checkreturndata()private pure returns(bool){
    bool success;
   // bool returndata;
    assembly {
        switch returndatasize()
            case 0 {                      // This is a non-standard ERC-20
                success := not(0)          // set success to true
            }
            case 32 {                     // This is a complaint ERC-20
                returndatacopy(0, 0, 32)
                success := mload(0)        // Set `success = returndata` of external call
            }
            default {                     // This is an excessively non-compliant ERC-20, revert.
                revert(0, 0)
            }
    }
    return success;
}

}

3. as you can see the transferfrom function in the simpleerc20 will return true

4. interact with the test contract by input the address with the address of the simpleerc contract, through do transferout function and dotransferoutwithcheck function, dotransferoutwithcheck is the same as https://github.com/code-423n4/2021-08-notional/blob/main/contracts/internal/balances/TokenHandler.sol#L221-260 with modified so it will return the success

5. and from the 2 of those function you can see that the dotransferout will return true, because the assembly instruction is inside the code, but the dotransferoutwithcheck will return false, because the assembly instruction is happing ouside the code.

Tools Used

remix

Recommended Mitigation Steps

move the checking inside the safetransferin and safetransferout function

[jeffywu]

I'm not sure that this bug report is correct, but it is a little hard to read. It seems unlikely that this bug report is valid because if it were true then none of the unit tests would pass at all. There is another issue that I discovered while trying to reproduce this issue. The safeTransferOut and safeTransferIn methods use the IERC20 interface from OpenZeppelin which says that solidity should decode a boolean return code.

For non-compliant ERC20 tokens there is no return code and therefore Solidity will revert trying to decode a return value that is not there. Since I took this assembly code from Compound: https://github.com/compound-finance/compound-protocol/blob/master/contracts/CErc20.sol#L192-L211

I discovered that I was using the wrong interface specification for the transfer and transferFrom call. The correct fix is to change the call to use a different interface specification so Solidity does not revert when trying to decode an empty return value.

This is a snipped from the required change:

    function safeTransferOut(
        address token,
        address account,
        uint256 amount
    ) private {
        IEIP20NonStandard(token).transfer(account, amount);
        checkReturnCode();
    }

    function safeTransferIn(
        address token,
        address account,
        uint256 amount
    ) private {
        IEIP20NonStandard(token).transferFrom(account, address(this), amount);
        checkReturnCode();
    }

Given that these non standard ERC20 tokens are probably quite rare at this point and we don't plan to list any of these I think this can be downgraded to medium or low severity. Also I'm not sure the actual report is correct.

https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

[jeffywu]

Ok I found the bug in his bug report :)

He does not return the boolean in this method:

function doTransferOutwithcheck(address token) public returns (bool) {
    simpleerc token = simpleerc(token);
    token.transferFrom();
    checkreturndata();   
}

Therefore it returns false always. Adding a return statement to the last line has it match the other method.

[jeffywu]

So his report is incorrect but technically there is an issue, duplicated in #80.

[ghoul-sol]

Per sponsor comment, making this invalid.