FeeWrapper fails to handle ETH payment refunds

[code423n4]

Lines of code

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

Vulnerability details

Impact

The FeeWrapper contract can be used to wrap calls that include ETH payments. This is handled by the _rubicallPayable function:

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:     }

As we can see in the previous snippet, the implementation will forward the ETH payment (minus fees) to the target contract. If the target contract ends up using less ETH than the sent amount, then the usual approach would be to refund the remaining ETH back to the caller, which is a normal and common operation.

If this is the case, then the wrapped call will fail as the FeeWrapper doesn't implement the receive or fallback function to allow ETH payments. Even though there is no loss of funds as the transaction is reverted, the issue will prevent users from wrapping calls to target contracts that may refund ETH as part of their normal behavior.

As a potential real example, we can the explore the buyAllAmountWithETH function present in the RubiconRouter contract:

https://github.com/RubiconDeFi/rubi-protocol-v2/blob/master/contracts/utilities/RubiconRouter.sol#L379-L418

379:     function buyAllAmountWithETH(
380:         ERC20 buy_gem,
381:         uint256 buy_amt,
382:         uint256 max_fill_amount
383:     ) external payable beGoneReentrantScum returns (uint256 fill) {
384:         address _weth = address(wethAddress);
385:         uint256 _before = ERC20(_weth).balanceOf(address(this));
386:         require(
387:             msg.value == max_fill_amount,
388:             "must send as much ETH as max_fill_amount"
389:         );
390:         IWETH(wethAddress).deposit{value: max_fill_amount}(); // Pay with native ETH -> WETH
391: 
392:         if (
393:             IWETH(wethAddress).allowance(address(this), RubiconMarketAddress) <
394:             max_fill_amount
395:         ) {
396:             approveAssetOnMarket(wethAddress);
397:         }
398: 
399:         // An amount in WETH
400:         fill = RubiconMarket(RubiconMarketAddress).buyAllAmount(
401:             buy_gem,
402:             buy_amt,
403:             ERC20(wethAddress),
404:             max_fill_amount
405:         );
406:         IERC20(buy_gem).safeTransfer(msg.sender, fill);
407: 
408:         uint256 _after = ERC20(_weth).balanceOf(address(this));
409:         uint256 delta = _after - _before;
410: 
411:         // Return unspent coins to sender
412:         if (delta > 0) {
413:             IWETH(wethAddress).withdraw(delta);
414:             // msg.sender.transfer(delta);
415:             (bool success, ) = msg.sender.call{value: delta}("");
416:             require(success, "Transfer failed.");
417:         }
418:     }

As we can see in the previous snippet, the function will potentially refund the caller the unspent ETH in lines 412-417. If this call is being wrapped using the FeeWrapper, then msg.sender will be the FeeWrapper contract.

Proof of Concept

In the following test we create a demonstration contract FeeWrapperTarget which includes a function named demoETH that will refund half of the sent amount back to the caller. The wrapped call will fail, as the FeeWrapperTarget will try to refund the ETH to the FeeWrapper which doesn't allow ETH payments, causing the whole transaction to be reverted.

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

function test_FeeWrapper_FailsWithETHRefund() public {
    FeeWrapperTarget target = new FeeWrapperTarget(ERC20(address(0)));

    uint256 amount = 1 ether;
    uint256 fee = 0.1 ether;

    vm.deal(alice, amount + fee);

    // Alice will call demoERC20
    vm.startPrank(alice);

    FeeWrapper.CallParams memory callParams;
    callParams.selector = FeeWrapperTarget.demoETH.selector;
    callParams.args = "";
    callParams.target = address(target);
    callParams.feeParams.feeToken = address(0);
    callParams.feeParams.totalAmount = amount + fee;
    callParams.feeParams.feeAmount = fee;
    callParams.feeParams.feeTo = makeAddr("FeeRecipient");

    // The following call will fail, the FeeWrapper contract is not prepared to receive the ETH from the target contract
    vm.expectRevert("low-level call to the router failed");
    feeWrapper.rubicall{value: amount + fee}(callParams);

    vm.stopPrank();
}

Recommendation

Allow the FeeWrapper contract to receive ETH by implementing the receive function. After the call to the target contract, refund any ETH amount present in the FeeWrapper contract back to the original caller.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1283

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[HickupHH3]

Core issue is that the fee wrapper lacks functionality to handle refunds in general, whether in ETH or ERC20. Specifically, for ETH, lacking receive() / fallback() functions to accept inbound transfers.

Generally, I would've considered this to be M severity because it's conditional on the target contract sending back funds. However, the fee wrapper is expected to interact with the Rubicon contracts, which the warden has shown to have the ability send back funds. Hence, the H severity is justified.

[c4-judge]

HickupHH3 changed the severity to 3 (High Risk)

[c4-judge]

HickupHH3 marked the issue as duplicate of #1283

[c4-judge]

HickupHH3 marked the issue as selected for report