QA Report

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/compound_rari_fork/CToken.sol#L1718 https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/compound_rari_fork/CErc20.sol#L179 https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/compound_rari_fork/CErc20.sol#L205 https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/compound_rari_fork/CErc20.sol#L217

Vulnerability details

Impact

this bug is also well-known. i notice that cERC20.sol, doTransferIn and doTransferOut all use the _callOptionalReturn func to do the heavy lift. but, when dig inside the _callOptionalReturn func, it use the _functionCall func to transfer tokens. but it doesn't check the token address must be contract address. if the target is an EOA, it also pass. this bug leads to the infamous Qubit hack. also first pointed out by Samczsun for the 0x protocol. you may refer to samczsun's link to check it better. https://samczsun.com/the-0x-vulnerability-explained/

Proof of Concept

function doTransferOut(address payable to, uint256 amount) internal {
        _callOptionalReturn(
            abi.encodeWithSelector(EIP20NonStandardInterface(underlying).transfer.selector, to, amount),
            "TOKEN_TRANSFER_OUT_FAILED"
        );
    }
function _callOptionalReturn(bytes memory data, string memory errorMessage) internal {
        bytes memory returndata = _functionCall(underlying, data, errorMessage);
        if (returndata.length > 0) require(abi.decode(returndata, (bool)), errorMessage);
    }
function _functionCall(
        address target,
        bytes memory data,
        string memory errorMessage
    ) internal returns (bytes memory) {
        (bool success, bytes memory returndata) = target.call(data);

}
===>
just call doTransferOut(address(0), amount);

Tools Used

manual

Recommended Mitigation Steps

function _functionCall(
        address target,
        bytes memory data,
        string memory errorMessage
    ) internal returns (bytes memory) {
        require(isContract(target), "Address: delegate call to non-contract");
        (bool success, bytes memory returndata) = target.call(data);

[0xdramaone]

Seems unlikely to be of actual risk given that it would require the deployment of a CToken with underlying to be a non-contract to be at all relevant, but simple enough to add to decrease potential surface area

[jack-the-pug]

Downgraded to QA as it poses no practical threat.

[JeeberC4]

Generating QA Report as warden did not submit on and judge downgraded issue. Preserving original title: when using safe transfer token, it is better to check the address to must be contract