Upgradeability of collateral assets opens up the doors to reentrancy vulnerabilities

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L148-L154
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L214-L224

Vulnerability details

This issue revisits reentrancy concerns that were raised in previous audits of the protocol (Issue #297, Issue #347, Issue #95, Issue #304). While the protocol team initially decided to mitigate these concerns by not supporting ERC-777 assets, this approach does not work if the protocol aims to be secure against arbitrary upgrades of collateral assets as per the in-scope ERC-20 behaviours.

The referenced findings remain largely relevant to the current version of the protocol. The RToken contract, for instance, is vulnerable to reentrancy via external token call in the functions issue()/issueTo() and redeem()/redeemTo(). These functions interact with multiple ERC20 tokens that make up the collateral basket.

If any of the tokens in the basket were to implement transfer hooks through an upgrade, an attacker could exploit this vulnerability to reenter the protocol in an inconsistent state.

For example, in the RToken.issue() function:

function issue(address recipient, uint256 amount) external {
    // ... checks and effects ...
    _mint(recipient, amtRToken);
    
    for (uint256 i = 0; i < erc20s.length; ++i) {
        IERC20Upgradeable(erc20s[i]).safeTransferFrom(
            issuer,
            address(backingManager),
            deposits[i]
        );
    }
}

If an attacker obtained control flow on any of the transferFrom() calls to the erc20s, they could call back into the protocol before all transfers are complete, allowing them to steal funds as per the findings referenced above.

Impact

Theft of funds

Proof of Concept

1. A collateral token is upgraded to implement ERC-777, or simply hacked
2. Attacker calls RToken.issue() with a large amount
3. During the token transfer, the upgraded token's tokensToSend hook is triggered
4. Inside this hook, the attacker calls BackingManager.manageTokens()
5. manageTokens() sees an inconsistent state where RToken.basketsNeeded has been updated but not all collateral has been transferred
6. This leads to incorrect calculations in manageTokens(), potentially allowing the attacker to extract excess collateral

Tools Used

Manual review

Recommended Mitigation Steps

As pointed out by the sponsor in this comment, the multi-contract architecture of the protocol would require a global reentrancy mutex to fully protect against this class of vulnerabilities. Nevertheless, reentrancy protection at the contract level may prove sufficient for many such instances.

Assessed type

Upgradable

[akshatmittal]

We explicitly do not support ERC-777 (and other similar standards), including upgrading to those standards. While "upgradable" ERC-20 are supported, it does not mean upgrading to ERC-777 is on the table. A token moving from ERC-20 to ERC-777 is not a small change.

[c4-judge]

thereksfour marked the issue as unsatisfactory:
Out of scope