REENTRANCY ATTACK POSSIBLE IF THE _feeTo IS A MALICIOUS CONTRACT IN FeeWrapper._chargeFeePayable() FUNCTION

[code423n4]

Lines of code

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

Vulnerability details

Impact

Reentrancy attack could happen if _feeTo is a malicious contract and has recieve() function implemented in _feeTo contract. FeeWrapper._chargeFeePayable() function is used to send the _feeAmount of Eth to the _feeTo address.

Consider that FeeWrapper._chargeFeePayable() is called by the FeeWrapper._rubicallPayable() function with the necessary _feeParams input parameter, and the following scenario can occur:

1 . The call to (bool OK, ) = payable(_feeTo).call{value: _feeAmount}("") is made. Lets assume the _totalAmount = 1Eth and _feeAmount = 0.1Eth initially.
2 . _feeTo contract has recieve() function implemented in it.
3 . It reenters the FeeWrapper contract by calling the rubicallPayable() function by sending 0.01Eth as msg.value.
4 . Sets the _feeParams.totalAmount = 0.01Eth and _feeParams.feeAmount = 0.1Eth in the call to rubicallPayable()
5 . FeeWrapper.chargeFeePayable() will be called again by the rubicallPayable() function.
6 . (bool OK, ) = payable(_feeTo).call{value: _feeAmount}(""); will be called and Tx will pass since there is enouth Eth in the contract from the previous deposit.
7 . Again _feeParams.feeAmount = 0.1Eth will be transferred to the _feeTo address.
8 . This can be continued till there is enough Eth in the contract.
9 . This happens due to there is no check to make sure msg.value > _feeAmount before the low level call to the _feeTo address.

Proof of Concept

function _chargeFeePayable(
    FeeParams memory _feeParams
) internal returns (uint256 _msgValue) {
    // _feeToken is ETH
    uint256 _totalAmount = _feeParams.totalAmount;
    uint256 _feeAmount = _feeParams.feeAmount;
    address _feeTo = _feeParams.feeTo;
    require(msg.value == _totalAmount, "FeeWrapper: not enough ETH sent");

    // transfer fee to the 3rd party protocol
    (bool OK, ) = payable(_feeTo).call{value: _feeAmount}("");
	require(OK, "ETH transfer failed");
    _msgValue = msg.value - _feeAmount;
}

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

function _rubicallPayable(
    CallParams memory _params
) internal returns (bytes memory) {
    // charge fee from feeParams
    uint256 _msgValue = _chargeFeePayable(_params.feeParams);

    (bool _OK, bytes memory _data) = _params.target.call{value: _msgValue}(
        bytes.concat(_params.selector, _params.args)
    );

    require(_OK, "low-level call to the router failed");

    return _data;
}

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Two options can be recommended to mitigate this issue:

1. Add a reentrancy guard. Openzeppelin ReentrancyGuard.sol can be used, and nonReentrant modifier can be added to both FeeWrapper._chargeFeePayable() and FeeWrapper._rubicallPayable() functions.

2. Check that the ``msg.value > _feeAmountbefore the low levelcall` to the `_feeTo` address is made. In that case there is no gain to the `_feeTo` malicious contract, since the value it sending via `msg.value` is any way greater than the `_feeAmount` which he can steal via `reentrancy attack`.

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #495

[c4-judge]

HickupHH3 marked the issue as selected for report