Cross-Function Reentrancy Vulnerability Leading to Unintended Token Minting in `RewardableERC20Wrapper.deposit`

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/erc20/RewardableERC20Wrapper.sol#L41-L49

Vulnerability details

Impact

The RewardableERC20Wrapper contract, specifically the deposit() function, presents a significant vulnerability to both direct and cross-function reentrancy attacks. This vulnerability arises when an ERC777 token or another token type with "hook" functionality is used as the underlying token. A successful attack could lead to undesired token minting, which could then result in a distorted token supply, affecting the contract's overall integrity and the value of individual tokens.

Proof of Concept

The deposit() function in question, as currently inherited by CTokenWrapper.sol, CurveGaugeWrapper.sol, and StargateRewardableWrapper.sol, and probably more other asset plugin contracts in the future, is:

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/erc20/RewardableERC20Wrapper.sol#L41-L49

    /// Deposit the underlying token and optionally take an action such as staking in a gauge
    function deposit(uint256 _amount, address _to) external virtual {
        if (_amount > 0) {
            _mint(_to, _amount); // does balance checkpointing
            underlying.safeTransferFrom(msg.sender, address(this), _amount);
            _afterDeposit(_amount, _to);
        }
        emit Deposited(msg.sender, _to, _amount);
    }

In this function, _mint(_to, _amount) is executed before underlying.safeTransferFrom(msg.sender, address(this), _amount). An ERC777 token (or other token with similar hook capabilities) could trigger a reentrant call back to deposit(), leading to a sequence of operations where tokens get minted before the contract's state is correctly updated.

Moreover, a cross-function reentrancy attack can exploit this vulnerability. Consider a scenario where another function foo() in the contract calls underlying.safeTransferFrom(). The execution flow could proceed as follows:

1. foo() is called and invokes underlying.safeTransferFrom().
2. An ERC777 token contract is called, which then reenters the deposit() function.
3. The deposit() function mints tokens and attempts to transfer tokens from msg.sender to the contract.
4. Since the state of the contract is not updated until foo() finishes executing, an attacker could manipulate the contract into minting more tokens than they should receive.

This vulnerability highlights the importance of the check-effects-interactions pattern in contract development. Merely using a reentrancy guard may not prevent such sophisticated attacks. In the presence of nonReentrant visibility, the function could at least be exploited once where the cross-function reentrancy attacker ended up getting one free mint for whatever amount that is possible.

Tools Used

Manual

Recommended Mitigation Steps

1. Add a reentrancy guard still to all public and external functions that modify the contract's state and call external contracts. This can be achieved by utilizing OpenZeppelin's ReentrancyGuard contract and the nonReentrant modifier.

2. Refactor the deposit function to follow the check-effects-interactions pattern, where state changes are made after any external calls. The refactored function could look like this:

    /// Deposit the underlying token and optionally take an action such as staking in a gauge
    function deposit(uint256 _amount, address _to) external virtual {
        if (_amount > 0) {
-            _mint(_to, _amount); // does balance checkpointing
            underlying.safeTransferFrom(msg.sender, address(this), _amount);
            _afterDeposit(_amount, _to);
+            _mint(_to, _amount); // does balance checkpointing
        }
        emit Deposited(msg.sender, _to, _amount);
    }

Assessed type

Reentrancy

[thereksfour]

No actual attack scenarios

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)