Low level calls to accounts with no code will succeed in FeeWrapper

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/utilities/FeeWrapper.sol#L60-L73
https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/utilities/FeeWrapper.sol#L76-L89

Vulnerability details

Impact

The FeeWrapper contract wraps calls to an arbitrary target contract to add support for fees. The implementation executes a low level call to proxy the call to the target contract. This can be seen in the _rubicall and _rubicallPayable functions:

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/utilities/FeeWrapper.sol#L60-L73

60:     function _rubicall(
61:         CallParams memory _params
62:     ) internal returns (bytes memory) {
63:         // charge fee from feeParams
64:         _chargeFee(_params.feeParams, _params.target);
65: 
66:         (bool _OK, bytes memory _data) = _params.target.call(
67:             bytes.concat(_params.selector, _params.args)
68:         );
69: 
70:         require(_OK, "low-level call to the Rubicon failed");
71: 
72:         return _data;
73:     }

https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/utilities/FeeWrapper.sol#L76-L89

76:     function _rubicallPayable(
77:         CallParams memory _params
78:     ) internal returns (bytes memory) {
79:         // charge fee from feeParams
80:         uint256 _msgValue = _chargeFeePayable(_params.feeParams);
81: 
82:         (bool _OK, bytes memory _data) = _params.target.call{value: _msgValue}(
83:             bytes.concat(_params.selector, _params.args)
84:         );
85: 
86:         require(_OK, "low-level call to the router failed");
87: 
88:         return _data;
89:     }

Low level calls behave differently than function calls in Solidity. Calls at the EVM level to accounts with no code are successful, this is the expected and normal behavior. It is Solidity that adds checks to prevent accidental calls to accounts with no code while compiling code for normal function calls.

This means that if the target account has no code, then the wrapped call using the FeeWrapper contract will succeed and the operation will be executed successfully. An accidental mistake may go unnoticed and may also cause unexpected loss of funds, as this call may include call value for transferring ETH.

Proof of Concept

In the following test, we use the FeeWrapper contract to execute a call to an account with no code. The transaction will succeed and the ETH will be transferred to the target account.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_FeeWrapper_CallsToAccountsNoCodeSucceed() public {
    FeeWrapper.CallParams memory callParams;
    callParams.selector = bytes4(0x01020304);
    callParams.args = abi.encode(uint256(42));
    callParams.target = makeAddr("address with no code");
    callParams.feeParams.totalAmount = 0.1 ether;
    callParams.feeParams.feeAmount = 0.01 ether;
    callParams.feeParams.feeTo = makeAddr("FeeRecipient");

    // Target has no code
    assertEq(callParams.target.code.length, 0);

    // Call succeeds even though the target has no code and no implementation
    feeWrapper.rubicall{value: 0.1 ether}(callParams);
}

Recommendation

While executing low level calls, the _rubicall and _rubicallPayable functions should check that either the account has code or the return data is greater than zero (which indicates the presence of an implementation). The OpenZeppelin contracts library provides utilities to execute low level calls in a safe way, including the recommended checks, present in the Address library, functionCall and functionCallWithValue.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

daoio marked the issue as sponsor confirmed

[HickupHH3]

Value lost would be msg.value, preventing it has a low cost, sticking to M.
Ref: code-423n4/org#53 (comment)

[c4-judge]

HickupHH3 marked the issue as selected for report