Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Reserve Core smart contract system written in Solidity. The audit took place between July 29—August 19 2024.

Following the C4 audit, 4 wardens (RadiantLabs (3docSec and EV_om), ether_sky and Bauchibred) reviewed the mitigations for all identified issues; the mitigation review report is appended below the audit report.

Wardens

19 Wardens contributed reports to Reserve Core:

1. RadiantLabs (3docSec and EV_om)
2. krikolkk
3. ether_sky
4. Bauchibred
5. stuart_the_minion
6. Shield (Viraz, 0xA5DF, Dravee, Udsen, and ElGreenGoblino)
7. SUPERMAN_I4G
8. Agontuk
9. PolarizedLight (ChaseTheLight and Auditor_Nate)
10. Rhaydden
11. DanielArmstrong
12. 0x52
13. Aamir

This audit was judged by cccz.

Final report assembled by liveactionllama and thebrittfactor.

Summary

The C4 analysis yielded an aggregated total of 7 unique vulnerabilities. Of these vulnerabilities, 0 received a risk rating in the category of HIGH severity and 7 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 12 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Reserve Core repository, and is composed of 34 smart contracts written in the Solidity programming language and includes 4,079 lines of Solidity code.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

Medium Risk Findings (7)

[M-01] RToken can manipulate distribution to avoid paying DAO fees

Submitted by RadiantLabs

Revenue produced by RTokens is sold for both RSR and RTokens according to a distribution defined in the Distributor. The BackingManager splits the collateral tokens to be sold proportionately to the RSR/RToken distribution ratio and sends them to the rsrTrader and rTokenTrader. When trades settle, the obtained RSR or RTokens are sent to the Distributor, which distributes them no longer according to the RSR/RToken ratio but to the different destinations for the specific token. The sum of all destinations for each token is used to derive the ratio.

The DAO fee is added to the RSR share of the initial split and paid when RSR is distributed.

However, the current implementation allows governance to manipulate the distribution settings without much effort in a way that can significantly reduce the amount of DAO fees paid.

This can be achieved through a combination of two different root causes:

• an RSR destination can be added that prevents rewards from being immediately distributed
• the RSR/RToken ratio is calculated twice: once in the BackingManager, and once in the Distributor, and it is can be modified between the two

Essentially, the distribution can be set in a way that temporarily accumulates RSR revenue in in the rsrTrader according to one RSR/RToken ratio, and then later redistributed with a different ratio.

Impact

RTokens can avoid paying most of the DAO fee

Proof of Concept

Assuming a 10% DAO fee, governance can execute the following steps to pay only about 1% in fees:

1. Set the distribution such that rsrTotal == 0 before DAO fees are added.
2. Add the RSR token itself as a distribution target. Like stRSR, RSR does not allow transferring to itself, so the distribution will always fail.
3. 10% of revenue will accumulate as RSR in the rsrTrader.
4. After some time, change the distribution such that rTokenTotal == 0 and add another destination with non-zero rsrDist (e.g., stRSR).
5. Remove RSR as a distribution target and call rsrTrader.distributeTokenToBuy().
6. Only 10% of the accumulated RSR will go to the DAO, which is effectively 1% of the total revenue.
7. Repeat this process as needed.

Recommended Mitigation Steps

Disallowing RSR as a distribution token prevents this to a large extent.

cccz (judge) commented:

Will try to get the sponsor’s opinion, and will set it to valid before the sponsor responds.
It’s about Malicious Governance, though it actually compromises the DAO, so it’s probably a privilege escalation.

akshatmittal (Reserve) disputed and commented:

This is a known issue, changing Distributions in a specific way can change what is actually paid out to veRSR. See publicly known issues as well as the Trust report specifically.

tbrent (Reserve) commented:

The way I’m seeing it: a new finding (RSR self-entries in the distributor table cause distribution to revert) raises the previously known issue TRST-L-2 in severity from Low to Medium, due to what is effectively privilege escalation by avoiding paying the DAO fee.

Reserve mitigated

Status: Mitigation confirmed. Full details in reports from Bauchibred, ether_sky and RadiantLabs.

[M-02] Broken assumptions can lead to the inability to seize RSR

Submitted by krikolkk

The seizeRSR function takes RSR from the staking contract when BackingManager wants to sell RSR, but it does not have enough. In this case, the stakers can lose a portion of their stake in order to keep the system healthy. However, an issue arises from broken assumptions about stakeRSR and totalStakes, which will make the contract unable to seize due to revert.

Proof of Concept

Let’s assume the system becomes unhealthy, and the stakers will start unstaking their tokens in fear of an upcoming reset/seize. Then, the expected event comes, and the BackingManager tries to seize RSR from the staking contract.

This action is frontran however, with the last staker who:

1. unstakes everything
2. stakes one token
3. stakes one token again

Let’s examine how does this break seizeRSR.

1. During the unstake call:

   • _payoutRewards is called. Since totalStakes is still above 1e18, stakeRSR can be increased and become greater than totalStakes. Let’s assume that totalStakes would be 1000e18 and stakeRSR would end at 1001e18 after this step.

   • Now, the stakeRate would be updated. Since stakeRSR is not zero yet, and so is not totalStakes, it would be set to

       uint192((totalStakes * FIX_ONE_256 + (stakeRSR - 1)) / stakeRSR)

     which translates to

       uint192((1000e18 * 1e18 + (1001e18 - 1)) / 1001e18)

     resulting in 999000999000999001

   • totalStakes would be decreased in _burn by 1000e18 to zero, which would result in stakeRSR being set to zero

2. During the stake call:

   • payoutRewards is skipped because it was already called in this block
   • newTotalStakes in mintStakes is calculated as (stakeRate * newStakeRSR) / FIX_ONE, which translates to (stakeRate * (stakeRSR + rsrAmount)) / FIX_ONE which translates to (999000999000999001 * (0 + 1)) / 1e18 resulting in 0, meaning 0 tokens are minted and totalStakes remains 0
   • stakeRSR is increased by 1

3. The second stake call will have the same effects

   • after this call, the stakeRate is 999000999000999001, totalStakes is 0, and stakeRSR is 2 (note that this breaks this assumption)

4. Now we examine the important parts of the seizeRSR call

   • stakeRSRToTake is set to (2 * rsrAmount + (rsrBalance - 1)) / rsrBalance, which will result in 1 every time the we are not seizing more than half of the total balance of the contract
   • stakeRSR is therefore set to 1, and since it is non-zero, stakeRate is updated to uint192((FIX_ONE_256 * totalStakes + (stakeRSR - 1)) / stakeRSR), which translates to uint192((1e18 * 0 + (1 - 1)) / 1) which comes to 0

   • the rest of the call is not important until it comes to

     emit ExchangeRateSet(initRate, exchangeRate());

   • Here, we can see that the call returns

     return (FIX_SCALE_SQ + (stakeRate / 2)) / stakeRate;

   • since stakeRate was updated to 0 before, the call will revert

Impact and Likelihood

The likelihood of this issue is LOW. The impact seems somewhere between HIGH and MEDIUM since a necessary rebalance action can be DoSed until it is noticed and an action is taken. Considering that the issue is possible due to a broken invariant, the severity should be judged as MEDIUM.

Recommendation

One way to fix the issue would be to enforce the invariant if totalStakes == 0, then stakeRSR == 0. This could be done by assuring that amount is not 0 in _mint.

    function _mint(address account, uint256 amount) internal virtual {
        _notZero(account);
+       _notZero(amount);
        assert(totalStakes + amount < type(uint224).max);

        stakes[era][account] += amount;
        totalStakes += amount;

        emit Transfer(address(0), account, amount);
        _afterTokenTransfer(address(0), account, amount);
    }

Another mitigation could be to update the seizeRSR function to update the stakeRate only if both stakeRSR and totalStakes are non-zero or update the stakeRate to FIX_ONE if either of these two is zero.

tbrent (Reserve) commented:

This is a plausible issue. We would like to request a coded PoC from the warden, since it is about a very specific numeric case.

cccz (judge) commented:

Since the initial judging time is not enough to verify it, consider it valid first and keep open, and wait for the PoC from warden.
And may invalidate it without further proof.

akshatmittal (Reserve) commented:

Just noting that we currently think this is not possible but might be plausible in super specific circumstances which is why we’re requesting the PoC.

krikolkk (warden) commented:

Hi @akshatmittal @tbrent - The PoC in the issue is wrong; however, the issue exists. Below I attach the updated PoC with a test proving the issue. Under those conditions seizeRSR will be dosed due to a broken protocol invariant.

Let's assume the system becomes unhealthy, and the stakers will start unstaking their tokens in fear of an upcoming reset/seize. Then the following chain of action happens:

1. The last staker unstakes everything but one token so _payoutRewards updates stakeRSR
2. Stake rate is below 5e17 - this can happen natuarally or forcefully (in our scenario everybody unstaked so we can influence it in some way)
3. BackingManager tries to seize RSR from the staking contract, but this transaction is frontran with another transaction in which
4. The last staker unstakes the last token
5. The last staker stakes 2 wei

What will happen is that

1. During the unstake call:

   • _payoutRewards is called. Since totalStakes is still above 1e18, stakeRSR can be increased and become greater than totalStakes.
   • This will influence the new stakeRate. If this becomes less than 5e17, the problem arises

2. During the stake call:

   • payoutRewards is skipped because it was already called in this block
   • newTotalStakes in mintStakes is calculated as (stakeRate * newStakeRSR) / FIX_ONE, which translates to (stakeRate * (stakeRSR + rsrAmount)) / FIX_ONE which translates to (499999999999999999 * (0 + 2)) / 1e18 resulting in 0, meaning 0 tokens are minted and totalStakes remains 0
   • stakeRSR is increased by 2
   • Note that 499999999999999999 is the greatest possible value of stakeRate when the issue arises
   • in the end we can see that totalStakes is 0, and stakeRSR is 2 (note that this breaks this assumption)

3. Now we examine the important parts of the seizeRSR call

   • stakeRSRToTake is set to (2 * rsrAmount + (rsrBalance - 1)) / rsrBalance, which will result in 1 every time the we are not seizing more than half of the total balance of the contract
   • stakeRSR is therefore set to 1, and since it is non-zero, stakeRate is updated to uint192((FIX_ONE_256 * totalStakes + (stakeRSR - 1)) / stakeRSR), which translates to uint192((1e18 * 0 + (1 - 1)) / 1) which comes to 0

   • the rest of the call is not important until it comes to

     emit ExchangeRateSet(initRate, exchangeRate());

   • Here, we can see that the call returns

     return (FIX_SCALE_SQ + (stakeRate / 2)) / stakeRate;

   • since stakeRate was updated to 0 before, the call will revert here

The following test showcases the issue with different numbers:

it.only("Unable to seize RSR", async () => {
  const stakeAmt: BigNumber = bn("5000e18");
  const unstakeAmt: BigNumber = bn("4999e18");
  const one: BigNumber = bn("1e18");
  const rewards: BigNumber = bn("100e18");
  const seizeAmount: BigNumber = bn("2499e18");

  // 1. Stake
  await rsr.connect(addr1).approve(stRSR.address, stakeAmt);
  await stRSR.connect(addr1).stake(stakeAmt);

  // 2. Decrease stakeRate to ~5e17 - 1
  await rsr.connect(owner).mint(stRSR.address, rewards);
  await advanceToTimestamp((await getLatestBlockTimestamp()) + 1);
  await stRSR.payoutRewards();
  await advanceToTimestamp((await getLatestBlockTimestamp()) + 86400);
  await stRSR.payoutRewards();

  // 3. Unstake everything but 1e18
  await stRSR.connect(addr1).unstake(unstakeAmt);
  await advanceToTimestamp((await getLatestBlockTimestamp()) + 172800);

  // 4. Unstake the last token then stake 2 wei

  // everything must happen in 1 tx thats why we deploy `SeizeAttacker`
  const SeizeAttackerFactory = await ethers.getContractFactory("SeizeAttacker");
  let seizeAttacker = await SeizeAttackerFactory.deploy();

  // transfer stRSR to the seize attacker so it can unstake
  await stRSR.connect(addr1).transfer(seizeAttacker.address, one);
  await rsr.connect(owner).mint(seizeAttacker.address, 2);
  await seizeAttacker.doIt(stRSR.address, rsr.address, one);

  // 5. seize rsr fails
  await setStorageAt(stRSR.address, 256, addr1.address); // set addr1 as backing manager so we can call seize rsr easily
  await stRSR.connect(addr1).seizeRSR(seizeAmount);
});

We will see that the test will fail with the following output

  Error: VM Exception while processing transaction: reverted with panic code 0x12 (Division or modulo division by zero)

akshatmittal (Reserve) commented:

Hey @krikolkk, thanks for that. Can you please share the code for SeizeAttacker contract and replace the last call with impersonating BackingManager instead? (So I’m able to reproduce this)

krikolkk (warden) commented:

@akshatmittal - mb, forgot about that.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

interface IStRSR {
    function stake(uint256 rsrAmount) external;
    function unstake(uint256 stakeAmount) external;
}

interface RSR {
    function approve(address who, uint256 amount) external;
}

contract SeizeAttacker {

    function doIt(address stRSR,address rsr, uint amount) external {
        // unstake
        IStRSR(stRSR).unstake(amount);
        // approve
        RSR(rsr).approve(stRSR, 2);
        // stake 1
        IStRSR(stRSR).stake(2);
    }

}

In the test we set addr1 as backing manager so addr1 can call the method, the test should work.

akshatmittal (Reserve) commented:

Accepted.

Specifically, want to point out that the following must be true:

1. Last staker in the pool.
2. stakeRate is < 5e17 - 1
3. Alone has > minTradeVolume worth of RSR, or total drafts > minTradeVolume. (Acceptable Values)
4. Able to frontrun the seize call.

krikolkk (warden) commented:

@akshatmittal - the volume can be cumulated with other stakers who already unstaked. The main problem here is that we are breaking a protocol variant, as shown in the PoC, hence the code behaves unexpectedly.

Reserve mitigated

Status: Mitigation confirmed. Full details in reports from Bauchibred, RadiantLabs and ether_sky.

[M-03] The default Governor Anastasius is unable to call resetStakes

Submitted by krikolkk

The StRSR contract contains a function resetStakes, which is used to reset the staking of a specific RToken system, when the stake rate becomes unsafe. When this happens, the era of the staking contract is incremented, which practically resets the StRSR token. Since this is a sensitive action, only governance can call this function.

The Reserve team provides a default and recommended Governance contract with TimelockController, which handles the proposal creation and execution. The contract also ensures that proposals created in a past era can not be queued or executed in the current era since the voting conditions can differ between eras. However due to this check, it is impossible for the Governance contract to call resetStakes, since the function would increment the era, and the following check whether the proposal was proposed in the same era would not hold and revert the transaction.

Proof of Concept

The following test added to ZZStRSR.tests.ts supports the issue:

it.only("Impossible to reset stakes with Governor Anastasius", async () => {
  // Setup governance
  const ONE_DAY = 86400;
  const VOTING_DELAY = ONE_DAY;
  const VOTING_PERIOD = ONE_DAY * 3;
  const MIN_DELAY = ONE_DAY * 7;

  let GovernorFactory = await ethers.getContractFactory("Governance");
  let stRSRVotes = await ethers.getContractAt("StRSRP1Votes", stRSR.address);
  let TimelockFactory = await ethers.getContractFactory("TimelockController");
  let timelock = <TimelockController>(
    await TimelockFactory.deploy(MIN_DELAY, [], [], owner.address)
  );

  let governor = await GovernorFactory.deploy(
    stRSRVotes.address,
    timelock.address,
    VOTING_DELAY,
    VOTING_PERIOD, // voting period
    0, // threshold set to 0 just to showcase the issue
    0 // quorum percentage set to 0 just to showcase the issue
  );

  /////////////////////////////////////////
  ///                                   ///
  /// First step: update timelock roles ///
  ///                                   ///
  /////////////////////////////////////////

  const proposerRole = await timelock.PROPOSER_ROLE();
  const executorRole = await timelock.EXECUTOR_ROLE();
  const cancellerRole = await timelock.CANCELLER_ROLE();
  const adminRole = await timelock.TIMELOCK_ADMIN_ROLE();

  await timelock.grantRole(proposerRole, governor.address);
  await timelock.grantRole(executorRole, governor.address);
  await timelock.grantRole(cancellerRole, governor.address);
  await timelock.revokeRole(adminRole, owner.address);

  // Then we will update the owner to a new decentralized Governor Anastasius
  await main.connect(owner).grantRole(OWNER, governor.address);
  await main.connect(owner).renounceRole(OWNER, owner.address);

  //////////////////////////////////////////
  ///                                    ///
  /// Second step: MAKE THE RATES UNSAFE ///
  ///                                    ///
  //////////////////////////////////////////

  const stakeAmt: BigNumber = bn("1000e18");
  const addAmt1: BigNumber = bn("100e18");
  const addAmt2: BigNumber = bn("120e30");

  // Stake
  await rsr.connect(addr1).approve(stRSR.address, stakeAmt);
  await stRSR.connect(addr1).stake(stakeAmt);

  expect(await stRSR.exchangeRate()).to.equal(fp("1"));
  expect(await stRSR.totalSupply()).to.equal(stakeAmt);
  expect(await stRSR.balanceOf(addr1.address)).to.equal(stakeAmt);

  // Add RSR to decrease stake rate - still safe
  await rsr.connect(owner).transfer(stRSR.address, addAmt1);

  // Advance to the end of noop period
  await advanceToTimestamp((await getLatestBlockTimestamp()) + 1);
  await stRSR.payoutRewards();

  // Calculate payout amount
  const decayFn = makeDecayFn(await stRSR.rewardRatio());
  const addedRSRStake = addAmt1.sub(decayFn(addAmt1, 1)); // 1 round
  const newRate: BigNumber = fp(stakeAmt.add(addedRSRStake)).div(stakeAmt);

  // Payout rewards - Advance to get 1 round of rewards
  await setNextBlockTimestamp((await getLatestBlockTimestamp()) + 1);
  await expect(stRSR.payoutRewards()).to.emit(stRSR, "ExchangeRateSet");
  expect(await stRSR.exchangeRate()).to.be.closeTo(newRate, 1);
  expect(await stRSR.totalSupply()).to.equal(stakeAmt);
  expect(await stRSR.balanceOf(addr1.address)).to.equal(stakeAmt);

  // Add a large amount of funds - rate will be unsafe
  await rsr.connect(owner).mint(owner.address, addAmt2);
  await rsr.connect(owner).transfer(stRSR.address, addAmt2);

  // Advance to the end of noop period
  await setNextBlockTimestamp((await getLatestBlockTimestamp()) + 1);
  await stRSR.payoutRewards();

  // Payout rewards - Advance time - rate will be unsafe
  await setNextBlockTimestamp((await getLatestBlockTimestamp()) + 100);
  await expect(stRSR.payoutRewards()).to.emit(stRSR, "ExchangeRateSet");
  expect(await stRSR.exchangeRate()).to.be.gte(fp("1e6"));
  expect(await stRSR.exchangeRate()).to.be.lte(fp("1e9"));
  expect(await stRSR.totalSupply()).to.equal(stakeAmt);
  expect(await stRSR.balanceOf(addr1.address)).to.equal(stakeAmt);

  //////////////////////////////////////////////////////////////////////////////////////
  ///                                                                                ///
  /// Step 3: Now that the rates are unsafe, we can start a proposal to reset stakes ///
  /// We will have to delegate some votes in order for the proposal to succeed       ///
  ///                                                                                ///
  //////////////////////////////////////////////////////////////////////////////////////

  await stRSRVotes.connect(addr1).delegate(addr1.address);
  await advanceBlocks(2);

  // Proposal info
  let encodedFunctionCall =
    stRSRVotes.interface.encodeFunctionData("resetStakes");
  let proposalDescription = "Proposal #1 - Reset stakes";
  let proposalDescHash = ethers.utils.keccak256(
    ethers.utils.toUtf8Bytes(proposalDescription)
  );

  // Propose
  const proposeTx = await governor
    .connect(addr1)
    .propose(
      [stRSRVotes.address],
      [0],
      [encodedFunctionCall],
      proposalDescription
    );

  const proposeReceipt = await proposeTx.wait(1);
  const proposalId = proposeReceipt.events![0].args!.proposalId;

  // Proposal created
  expect(await governor.state(proposalId)).to.equal(ProposalState.Pending);

  // Advance time to start voting
  await advanceBlocks(VOTING_DELAY + 1);
  expect(await governor.state(proposalId)).to.equal(ProposalState.Active);

  await governor.connect(addr1).castVote(proposalId, 1);
  await advanceBlocks(1);

  // Advance time till voting is complete
  await advanceBlocks(VOTING_PERIOD + 1);
  expect(await governor.state(proposalId)).to.equal(ProposalState.Succeeded);

  // Queue proposal
  await governor
    .connect(addr1)
    .queue([stRSRVotes.address], [0], [encodedFunctionCall], proposalDescHash);

  // Check proposal state
  expect(await governor.state(proposalId)).to.equal(ProposalState.Queued);

  // Advance time required by timelock
  await advanceTime(MIN_DELAY + 1);
  await advanceBlocks(1);

  //////////////////////////////////////////////////////////////////////////////
  ///                                                                        ///
  /// The execution will revert because the era changes during the execution ///
  ///                                                                        ///
  //////////////////////////////////////////////////////////////////////////////

  await expect(
    governor
      .connect(addr1)
      .execute(
        [stRSRVotes.address],
        [0],
        [encodedFunctionCall],
        proposalDescHash
      )
  ).to.be.revertedWith("TimelockController: underlying transaction reverted");

  // We can see that the proposal is still queued
  expect(await governor.state(proposalId)).to.equal(ProposalState.Queued);
});

Impact and Likelihood

The impact of this issue is MEDIUM, as under usual conditions, an impactful governance action would be unavailable. Since the probability of the stake rates being over/under max/min safe stake rate is low (as per inline docs), but the probability of the issue taking place is high, the likelihood of this issue is judged MEDIUM, hence the MEDIUM severity of this issue.

Recommendation

Consider changing the order of super._execute and the startedInSameEra check in the Governance::_execute function:

    function _execute(
        uint256 proposalId,
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) internal override(Governor, GovernorTimelockControl) {
+       require(startedInSameEra(proposalId), "new era");
        super._execute(proposalId, targets, values, calldatas, descriptionHash);
-       require(startedInSameEra(proposalId), "new era");
    }

akshatmittal (Reserve) confirmed

Reserve mitigated

Status: Mitigation confirmed. Full details in reports from Bauchibred, ether_sky and RadiantLabs.

[M-04] Dutch auctions can fail to settle if any other collateral in the basket behaves unexpectedly

Submitted by RadiantLabs

When a Dutch auction that originated from the backing manager receives a bid, it calls BackingManager.settleTrade() to settle the auction immediately, which attempts to chain into another rebalance() call. This chaining is implemented using a try-catch block that attempts to catch out-of-gas errors.

However, this pattern is not safe because empty error data does not always indicate an out-of-gas error. Other types of errors also return no data, such as calls to empty addresses casted as contracts and revert / require statements with no error message.

The rebalance() function interacts with multiple external assets and performs several operations that can throw empty errors:

1. In basketsHeldBy(), which calls _quantity(), which in turn calls coll.refPerTok() (this function should in theory never revert, but in case it interacts with the underlying ERC20, its implementation may have been upgraded to one that does).
2. In prepareRecollateralizationTrade(), which calls basketRange(), which also calls _quantity().
3. In tryTrade() if a new rebalancing trade is indeed chained, which calls approve() on the token via AllowanceLib.safeApproveFallbackToMax(). This is a direct interaction with the token and hence cannot be trusted, especially if the possibility of upgradeability is considered.

If any of these operations result in an empty error, the auction settlement will fail. This can lead to the Dutch auction being unable to settle at a fair price.

Note: we have found this finding pointing out the very same issue in a previous audit, but this report highlights a different root cause in where the error originates.

Impact

Dutch auctions may fail to settle at the appropriate price or at all.

Proof of Concept

1. A Dutch auction is initiated for rebalancing collateral.
2. After some time, a bidder attempts to submit a bid at fair market value.
3. BackingManager.settleTrade() is called by the trade contract.
4. The rebalance() function is called within the try-catch block.
5. The underlying ERC-20 of one of the collateral assets in the basket has an empty revert clause that currently throws when one of its functions is called.
6. The catch block receives an empty error and reverts the transaction.

Recommended Mitigation Steps

Avoid usage of this pattern to catch OOG errors in any functions that cannot revert and may interact with external contracts. Instead, in such cases always employ the _reserveGas() pattern that was iterated on to mitigate previous findings (1, 2, 3) with a similar root cause. We have found no other instances in which this applies.

akshatmittal (Reserve) disputed and commented:

1. This is a known issue.
2. The ERC20 upgrade to return empty revert data on calling any of its functions seems a little far fetched.

EV_om (warden) commented:

We do not think this should be considered a known issue either, unless it was accepted in a previous competition or pointed out in an audit.

An empty revert in one function of a collateral asset being characterized as far-fetched is a little surprising, considering findings were accepted in previous Reserve audits for the same situation but the token reverting, consuming all gas and consuming a specific amount of gas.

All those findings concerned the ability to unregister a misbehaving asset, which we found to now be guaranteed. However, we found an asset misbehaving could also have the additional impact of preventing auctions from settling for a different asset. This same impact was accepted as valid for a different root cause here.

Again, an empty revert is nothing unusual and a simple require() with no error message will produce it.

We think this scenario is very much realistic and would like to kindly ask for it to be reassessed.

cccz (judge) commented:

@akshatmittal and @tbrent - This seems to be a possible upgrade, please take a look, thanks!

Again, an empty revert is nothing unusual and a simple require() with no error message will produce it.

akshatmittal (Reserve) commented:

Looking back at this again @cccz.

The first two statements which hinge on refPerTok reverting are not valid since we require refPerTok to not revert. If a collateral plugin does revert on it, it must be fixed and replaced. The third example however, the approve one, is where I can see the token revert causing issues.

I currently can not see any sane ERC20 reverting on an approve case with no message, however you may have better examples than I do. I still consider it highly unlikely, although if you do have examples to share I’ll consider them.

And honestly, I currently do not see how to do better. For a little more context on that, we want settle to start a new auction, which is why that revert exists there, and we can’t use the _reserveGas pattern here since the gas cost for rebalance is unbound.

RTokens are designed to be governance focused, and we already have the requirement for Governance to only include collaterals they absolutely trust (which is why you’d see all RTokens today use blue chip assets only).

If you absolutely must consider it valid, I’d probably bring it down to Low/QA given the requirements here, but also looking for your thoughts.

cccz (judge) commented:

@EV_om - If there’s no example, I’ll invalidate it because the assumption isn’t valid.

EV_om (warden) commented:

@cccz - USDT and BNB throw empty errors on reverts within approve(), for instance.

These are the two largest market cap ERC-20 tokens in existence - again, this is not some theoretical esoteric behaviour but a realistic scenario.

There may not be a better approach if the gas cost of rebalance() is unbounded as you say @akshatmittal. But lack of an immediate mitigation does not invalidate the issue/make it QA.

akshatmittal (Reserve) commented:

@EV_om - Both of the examples you have mentioned throw on zero, which is a case handled within the code. (Also just saying here, BNB isn’t technically a supported token for other reasons)

this is not some theoretical esoteric behaviour but a realistic scenario.

Believe me, I’m not trying to say so. I’m really trying to find a realistic case where an upgrade on the token makes it regress in a basic ERC20 function.

And yeah, I’m also not saying not having a mitigation invalidates the issue, but rather that the protocol has ways of dealing with such specific things like wrapping the tokens, etc. We already wrap tokens that we don’t like behaviours of, or tokens that have weird behaviours.

(Talking to cccz to accept this, just trying to get a better idea)

cccz (judge) commented:

Although the likelihood is low, the assumed token satisfies acceptable upgradability, will upgrade it to Medium.

[M-05] Users can dodge losses due to StRSR era changes with instant operations

Submitted by RadiantLabs

The StRSR contract implements an era-based wrapping of the stakeRate and draftRate exchange rates whenever these pass the maximum accepted value of MAX_STAKE_RATE and MAX_DRAFT_RATE; the effect of this action is that StRSR token holders see their balance reset (stakeRate reset) and/or all StRSR vesting withdraws are forgotten and the relative funds lost (draftRate reset).

From the code below, we can see that these two rates can be reset independently;

File: StRSR.sol
424:     function seizeRSR(uint256 rsrAmount) external {
---
440:         // Remove RSR from stakeRSR
441:         uint256 stakeRSRToTake = (stakeRSR * rsrAmount + (rsrBalance - 1)) / rsrBalance;
442:         stakeRSR -= stakeRSRToTake;
443:         seizedRSR = stakeRSRToTake;
444: 
445:         // update stakeRate, possibly beginning a new stake era
446:         if (stakeRSR != 0) {
447:             // Downcast is safe: totalStakes is 1e38 at most so expression maximum value is 1e56
448:             stakeRate = uint192((FIX_ONE_256 * totalStakes + (stakeRSR - 1)) / stakeRSR);
449:         }
450:         if (stakeRSR == 0 || stakeRate > MAX_STAKE_RATE) {
451:             seizedRSR += stakeRSR;
452:             beginEra();
453:         }
454: 
455:         // Remove RSR from draftRSR
456:         uint256 draftRSRToTake = (draftRSR * rsrAmount + (rsrBalance - 1)) / rsrBalance;
457:         draftRSR -= draftRSRToTake;
458:         seizedRSR += draftRSRToTake;
459: 
460:         // update draftRate, possibly beginning a new draft era
461:         if (draftRSR != 0) {
462:             // Downcast is safe: totalDrafts is 1e38 at most so expression maximum value is 1e56
463:             draftRate = uint192((FIX_ONE_256 * totalDrafts + (draftRSR - 1)) / draftRSR);
464:         }
465: 
466:         if (draftRSR == 0 || draftRate > MAX_DRAFT_RATE) {
467:             seizedRSR += draftRSR;
468:             beginDraftEra();
469:         }

… and we’d then argue that they will most likely happen at different times, at the very least because rewards contribute to the evolution of stakeRate only.

In the event only one of the two rates reaches the cap and undergoes a reset, stakers can dodge the bullet of collectivized losses, by:

• sandwiching the stakeRate wrapping with unstake and cancelUnstake calls
• sandwiching the draftRate wrapping with cancelUnstake and unstake calls

Impact

Users can avoid the collectivization of rate wrapping losses, and artificially get a share of any RSR left in the StRSR contract.

This latter quantity may be considerable, because while unlikely, it’s not impossible that the rate wrapping happens when little RSR is seized from an otherwise large RSR balance in the StRSR contract.

Proof of Concept

Alice is an StRSR token holder:

• she notices that a transaction will soon cause RSR to be seized and the stakeRate will wrap to 1.0
• she immediately initiates an unstake, so her funds are moved to the drafts bucket
• immediately after the transaction that wraps the stakeRate, she calls cancelUnstake
• Alice lost only the pro-rata RSR that was seized (which may be minimal) while others lost everything

A similar sequence can be performed to avoid losses on funds that are being unstaked by instead calling cancelUnstake first and unstake later.

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider wrapping both rates when at least one reaches the maximum.

tbrent (Reserve) acknowledged and commented:

The wrapping occurs at the MAX_STAKE_RATE or MAX_DRAFT_RATE, ie 1e27.

resetStakes() is available to governance above MAX_SAFE_STAKE_RATE, ie 1e24.

StRSR is unstable between these bounds, but the risk of it occurring is assumed low enough to be not worth further mitigating. See: https://github.com/reserve-protocol/protocol/blob/72fc1f6e41da01e733c0a7e96cdb8ebb45bf1065/contracts/p1/StRSR.sol#L490

/// The stake rate is unsafe when it is either too high or too low.

///     There is the possibility of the rate reaching the borderline of being unsafe,
///     where users won't stake in fear that a reset might be executed.
///     A user may also grief this situation by staking enough RSR to vote against any reset.
///     This standoff will continue until enough RSR is staked and a reset is executed.
///     There is currently no good and easy way to mitigate the possibility of this situation,
///     and the risk of it occurring is low enough that it is not worth the effort to mitigate.

The reported issue requires the same set of assumptions to get into this state, and then additional assumptions about the size of the next seizeRSR(). However, the impact trades off directly against the likelihood: the further the rate is away from the 1e27 bounds, the less likely this case is to occur. The closer to 1e27, the more clear it is to governance that they need to resetStakes(). And there are 3 orders of magnitude between 1e27 and 1e24 for this dynamic to play out.

We would like to acknowledge the issue but dispute severity to Low, given it is a subset of a case already assumed low enough probability to not be worth mitigating.

cccz (judge) decreased severity to Low

3docSec (warden) commented:

Hi @cccz and @tbrent - the require statement in resetStakes makes this function a viable option to mitigate only the issue in case of stakeRate being close to wrapping, but does not help with the other scenario presented by this issue - that is when draftRate wraps.

    function resetStakes() external {
        _requireGovernanceOnly();
        require(
            stakeRate <= MIN_SAFE_STAKE_RATE || stakeRate >= MAX_SAFE_STAKE_RATE,
            "rate still safe"
        );

        beginEra();
        beginDraftEra();
    }

Because draftRate normally goes only up with seizeRSR, and stakeRate instead goes up with seizeRSR but also goes down with _payoutRewards, it is still possible that draftRate grows close to MAX_STAKE_RATE while stakeRate is still below MAX_SAFE_STAKE_RATE. This scenario would prevent governance intervention, entering a situation where a draftRate wrapping alone is inevitable.

I would therefore ask you to reconsider the Medium severity, and potentially consider another mitigation option in allowing calls to resetStakes also when draftRate grows above MAX_SAFE_STAKE_RATE.

tbrent (Reserve) commented:

That is a good point: resetStakes() cannot be used when it is the draft rate that is out of bounds. I think there is a change to the code to make there of some variety.

Still, in order for the below to be true:

draftRate grows close to MAX_STAKE_RATE while stakeRate is still below MAX_SAFE_STAKE_RATE.

there must be at least 1000x historical appreciation of StRSR overall. This is an additional assumption beyond the initial assumptions of: (i) draftRate grows close to MAX_STAKE_RATE (requires consecutive nearly full-but-not-full seizures); (ii) can frontrun seizeRSR().

cccz (judge) increased severity to Medium and commented:

Based on the above conversation, users can frontrun resetStakes to avoid losses or get benefits. If this is correct, I would consider this issue to be low likelihood + high impact = Medium severity.

Reserve mitigated

Status: Mitigation confirmed. Full details in reports from ether_sky and Bauchibred.

[M-06] The time available for a canceled withdrawal should not impact future unstaking processes

Submitted by ether_sky, also found by Bauchibred

Lines of Code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L279
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L658
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSR.sol#L368

Impact

When stakers want to unstake their StRSR, they cannot withdraw RSR immediately.
Instead, each withdrawal enters a queue and will become available after the unstaking delay period.
This queue operates on a FIFO basis, meaning earlier withdrawals are processed before later ones.
Stakers can also cancel their withdrawals, and the RSR will be restaked immediately.
However, even canceled withdrawals can still impact future withdrawal requests.

Proof of Concept

Suppose the current unstakingDelay is set to 1 day, and a user decides to unstake some amount of StRSR at time T (line 279).

function unstake(uint256 stakeAmount) external {
    address account = _msgSender();
    require(stakes[era][account] >= stakeAmount, "insufficient balance");
    _payoutRewards();
    _burn(account, stakeAmount);

    uint256 newStakeRSR = (FIX_ONE_256 * totalStakes + (stakeRate - 1)) / stakeRate;
    uint256 rsrAmount = stakeRSR - newStakeRSR;
    stakeRSR = newStakeRSR;

279:    (uint256 index, uint64 availableAt) = pushDraft(account, rsrAmount);
}

This request enters a queue, which we assume is empty at this point, making the withdrawal available at T + 1 day (line 658).

function pushDraft(address account, uint256 rsrAmount)
    internal
    returns (uint256 index, uint64 availableAt)
{
    CumulativeDraft[] storage queue = draftQueues[draftEra][account];
    index = queue.length;  // index = 0

    uint192 oldDrafts = index != 0 ? queue[index - 1].drafts : 0; // 0
    uint64 lastAvailableAt = index != 0 ? queue[index - 1].availableAt : 0; // 0
658:    availableAt = uint64(block.timestamp) + unstakingDelay; // T + 1 day

    if (lastAvailableAt > availableAt) {
        availableAt = lastAvailableAt;
    }
    queue.push(CumulativeDraft(uint176(oldDrafts + draftAmount), availableAt));
}

Now, let’s assume the user cancels this withdrawal at time T for testing purposes. Instead of removing the withdrawal from the queue, we simply increase the first available ID, which is a good approach and is easy to implement (line 368).

function cancelUnstake(uint256 endId) external {
    uint256 firstId = firstRemainingDraft[draftEra][account];
    CumulativeDraft[] storage queue = draftQueues[draftEra][account];
    if (endId == 0 || firstId >= endId) return;

    require(endId <= queue.length, "index out-of-bounds");

368:    firstRemainingDraft[draftEra][account] = endId;
}

Even though the queue technically still contains one item (the canceled withdrawal request), we can consider the queue empty in terms of available withdrawals.

Later, the owner updates the unstakingDelay to 1 hour.
The user attempts to unstake again, expecting to withdraw just 1 hour later, which should be possible.
However, the canceled withdrawal still impacts this new request.

In line 654, the index is 1 , and in line 657, lastAvailableAt is set to T + 1 day due to the canceled withdrawal.
As a result, in line 658, the availableAt for the new withdrawal is calculated as T + 1 hour, but since this is less than T + 1 day, the new withdrawal is also pushed to be available 1 day later.

function pushDraft(address account, uint256 rsrAmount)
    internal
    returns (uint256 index, uint64 availableAt)
{
    CumulativeDraft[] storage queue = draftQueues[draftEra][account];
654:    index = queue.length;  // index = 1

    uint192 oldDrafts = index != 0 ? queue[index - 1].drafts : 0; // 0
657:    uint64 lastAvailableAt = index != 0 ? queue[index - 1].availableAt : 0; // T + 1 day
658:    availableAt = uint64(block.timestamp) + unstakingDelay; // T + 1 hour

    if (lastAvailableAt > availableAt) {
        availableAt = lastAvailableAt;
    }
    queue.push(CumulativeDraft(uint176(oldDrafts + draftAmount), availableAt));
}

This situation is clearly unfair to the user.

The availableAt of canceled withdrawals should not be considered when determining the availability of new withdrawals.

Please add below test to the test/ZZStRSR.test.ts

    describe('PushDraft Test', () => {
      it('Should use current unstakingDelay', async () => {
        // old unstakingDelay is 1 day
        const oldUnstakingDelay = 3600 * 24
        await stRSR.connect(owner).setUnstakingDelay(oldUnstakingDelay)  
        const amount: BigNumber = bn('100e18')
        await rsr.connect(addr1).approve(stRSR.address, amount)
        await stRSR.connect(addr1).stake(amount)
      
        const draftEra = 1
        const availableAtOfFirst = await getLatestBlockTimestamp() + oldUnstakingDelay + 1
        /**
         * Unstaking request enter a queue, and withdrawal become available 1 day later
         */
        await expect(stRSR.connect(addr1).unstake(amount))
          .emit(stRSR, 'UnstakingStarted')
          .withArgs(0, draftEra, addr1.address, amount, amount, availableAtOfFirst)
      
        /**
         * Cancel the unstaking to eliminate any pending withdrawals
         */
        await stRSR.connect(addr1).cancelUnstake(1)
      
        // new unstakingDelay is 1 hour
        const newUnstakingDelay = 3600
        await stRSR.connect(owner).setUnstakingDelay(newUnstakingDelay)  
      
        await rsr.connect(addr2).approve(stRSR.address, amount)
        await stRSR.connect(addr2).stake(amount)
      
        const availableAtOfFirstOfUser2 = await getLatestBlockTimestamp() + newUnstakingDelay + 1
        /**
         * Unstaking request enter a queue, and withdrawal become available 1 hour later for a second user
         */
        await expect(stRSR.connect(addr2).unstake(amount))
          .emit(stRSR, 'UnstakingStarted')
          .withArgs(0, draftEra, addr2.address, amount, amount, availableAtOfFirstOfUser2)
      
        /**
         * Although the first unstaking was canceled, its available time still impacts subsequent unstaking requests
         */
        await expect(stRSR.connect(addr1).unstake(amount))
          .emit(stRSR, 'UnstakingStarted')
          .withArgs(1, draftEra, addr1.address, amount, amount, availableAtOfFirst)
      })
    })

Recommended Mitigation Steps

function pushDraft(address account, uint256 rsrAmount)
    internal
    returns (uint256 index, uint64 availableAt)
{
    CumulativeDraft[] storage queue = draftQueues[draftEra][account];
    index = queue.length;  

    uint192 oldDrafts = index != 0 ? queue[index - 1].drafts : 0; 
    
-    uint64 lastAvailableAt = index != 0 ? queue[index - 1].availableAt : 0; 
+    uint64 lastAvailableAt = index != 0 && firstRemainingDraft[draftEra][account] < index ? queue[index - 1].availableAt : 0; 

    availableAt = uint64(block.timestamp) + unstakingDelay; 

    if (lastAvailableAt > availableAt) {
        availableAt = lastAvailableAt;
    }
    queue.push(CumulativeDraft(uint176(oldDrafts + draftAmount), availableAt));
}

akshatmittal (Reserve) commented:

Issue confirmed.

Although, want to downgrade to Low given the requirements, actions and impact.

cccz (judge) decreased severity to Low and commented:

Considering the low likelihood (owner reduces unstakingDelay, user cancels withdrawals before that) and the earlier the cancellation, the lower the impact, it will be downgraded to Low.

ether_sky (warden) commented:

Hi @cccz - thanks for your review.

Let me clarify the impact and likelihood again.
As seen in the code below, the minimum unstaking delay is 2 minutes, and the maximum is 1 year. There’s a significant difference between these two values.

uint48 private constant MIN_UNSTAKING_DELAY = 60 * 2; // {s} 2 minutes
uint48 private constant MAX_UNSTAKING_DELAY = 60 * 60 * 24 * 365; // {s} 1 year

Suppose the current unstaking delay is set to the 1 year.
All stakers are operating under this delay now.
For various reasons, the admin decides to reduce the unstaking delay to a shorter period, say 1 month.
Stakers will realize they can withdraw their funds after 1 month instead of 1 year.
They can cancel their existing withdrawal requests and attempt to unstake again.
However, the original 1-year delay still applies to all stakers, meaning they cannot withdraw their funds after just 1 month—even if they cancel their old withdrawals.
Their funds remain locked for 1 year.

Considering the low likelihood (owner reduces unstakingDelay, user cancels withdrawals before that) and the earlier the cancellation, the lower the impact, it will be downgraded to Low

Regarding the likelihood, users do not need to cancel their withdrawals before the owner changes the unstaking delay. Even if they cancel their withdrawals anytime after the delay is changed, the old delay still impacts all stakers.

As for the impact, this situation results in users’ funds being locked and creates a DoS issue.

Given these points, I believe the impact and likelihood are at least medium. I would appreciate it if you could reconsider this issue.

cccz (judge) increased severity to Medium and commented:

When the owner reduces unstakingDelay, the user will not be able to apply the latest unstakingDelay even if they cancel the previous withdrawal.
If this is correct, I tend to raise it to Medium.

Reserve mitigated

Status: Mitigation confirmed. Full details in reports from Bauchibred, ether_sky and RadiantLabs.

[M-07] The tradeEnd in BackingManager isn’t updating correctly

Submitted by ether_sky, also found by RadiantLabs and stuart_the_minion

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BackingManager.sol#L114
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BackingManager.sol#L166

Impact

In the BackingManager, we use the tradeEnd value for each type of trade to prevent next auction from occurring within the same block. We can find a comment in the code explaining this in line 114.

function rebalance(TradeKind kind) external nonReentrant {
    // DoS prevention:
114:    // unless caller is self, require that the next auction is not in same block
    require(
        _msgSender() == address(this) || tradeEnd[kind] < block.timestamp,
        "already rebalancing"
    );

This approach works correctly for Batch auctions. However, with Dutch auctions, the tradeEnd value can inadvertently block the next auction from starting for a certain period.

Proof of Concept

The maximum auction length can be up to 1 week.

uint48 public constant MAX_AUCTION_LENGTH = 60 * 60 * 24 * 7; // {s} max valid duration, 1 week

And the minimum warm-up period in the BasketHandler is 1 minute.

uint48 public constant MIN_WARMUP_PERIOD = 60; // {s} 1 minute

Suppose a Dutch auction is created in the BackingManager with a length of 1 week (7 days), starting at timestamp T. The tradeEnd for this auction type is set to T + 7 days in line 166.

function rebalance(TradeKind kind) external nonReentrant {
155:    if (doTrade) {

165:	    ITrade trade = tryTrade(kind, req, prices);
166:	    tradeEnd[kind] = trade.endTime(); // {s}

168:	}
}

Now, let’s consider a scenario where one of the assets in the basket temporarily falls into an IFFY status. At this point, the current status of the basket is also IFFY (line 323~325).

function status() public view returns (CollateralStatus status_) {
	uint256 size = basket.erc20s.length;
    if (disabled || size == 0) return CollateralStatus.DISABLED;
    
    for (uint256 i = 0; i < size; ++i) {
        CollateralStatus s = assetRegistry.toColl(basket.erc20s[i]).status();
323:        if (s.worseThan(status_)) {
324:            if (s == CollateralStatus.DISABLED) return CollateralStatus.DISABLED;
325:            status_ = s;
        }
    }
}

The auction is settled one day later, triggering the rebalance function (line 92).

function settleTrade(IERC20 sell) public override(ITrading, TradingP1) returns (ITrade trade) {
    delete tokensOut[sell];
    trade = super.settleTrade(sell); // nonReentrant

    if (_msgSender() == address(trade)) {        
92:        try this.rebalance(trade.KIND()) {} catch (bytes memory errData) {
        }
    }
}

The check at line 116 passes because the caller is the BackingManager itself. However, the BasketHandler status is still IFFY, meaning it isn’t ready for a new auction. As a result, the check at line 121 prevents the next auction from being created.

function rebalance(TradeKind kind) external nonReentrant {
    requireNotTradingPausedOrFrozen();
    assetRegistry.refresh();

    require(
116:        _msgSender() == address(this) || tradeEnd[kind] < block.timestamp,
        "already rebalancing"
    );

    require(tradesOpen == 0, "trade open");
    
121:    require(basketHandler.isReady(), "basket not ready");
    
    require(block.timestamp >= basketHandler.timestamp() + tradingDelay, "trading delayed");
}

The tradeEnd is still set to T + 7 days and the current time is now T + 1 day.

After another day, the asset’s status recovers to SOUND, and the BasketHandler status updates to SOUND as well.
After the warm-up period (1 minute in our scenario), the BasketHandler is ready, and the time is now T + 2 days + 1 minute.
At this point, we want to create a new Dutch auction for rebalancing.
However, we can’t create the next Dutch auction until T + 7 days due to the existing tradeEnd.

This delay clearly poses an issue.

Recommended Mitigation Steps

    function settleTrade(IERC20 sell) public override(ITrading, TradingP1) returns (ITrade trade) {
            delete tokensOut[sell];
            trade = super.settleTrade(sell); // nonReentrant

            if (_msgSender() == address(trade)) {
                try this.rebalance(trade.KIND()) {} catch (bytes memory errData) {
                    if (errData.length == 0) revert(); // solhint-disable-line reason-string
                }

    +			tradeEnd[kind] = uint48(block.timestamp);

            }
        }
    }

akshatmittal (Reserve) confirmed, but disagreed with severity and commented:

We’d like to bump this down to Low.

This is an issue, although does not impact protocol availability. The protocol can still function as expected using the other trading methods.

cccz (judge) commented:

After reconsidering the issue, I think this should be valid Medium. It indeed affects the availability of the protocol, which meets the Medium severity.

tbrent (Reserve) commented:

The issue can only impact DutchTrade because it is the only KIND that can have auctions that end before the endTime.

I only see the availability of the protocol being impacted if the GnosisTrade’ reportViolation() triggers, which would ultimately be the real cause of the loss of availability. GnosisTrade is supposed to be the fallback mechanism for trading.

Possibly relevant: the notions of disabling in the Broker are very different for dutch trade and batch trade:

• in dutch trades it is intended to detect if the protocol is trading off bad pricing data, since a dutch trade necessarily involves an assumption about the highest possible price. This is possible to trigger intentionally by burning $.
• in batch trades it is intended to detect if EasyAuction goes beyond the worst-case min buy amounts. Similar to if a Uniswap swap violated slippage constraints, it should not happen. This should not be possible to trigger intentionally by burning $.

ether_sky (warden) commented:

Thanks for your reveiw.

There are only two types of trades.
Because of this issue, Dutch auctions cannot occur for up to one week.
In some cases, Dutch auctions could be more efficient than Batch trades, but we are required to use Batch trades instead.

Additionally, as the sponsor described, Batch trades can be paused due to reportViolation().
In such cases, this issue could have a significant impact.

Therefore, I believe this issue deserves a Medium severity based on its impact and likelihood.

cccz (judge) commented:

Yes, as warden said, Dutch auctions being unavailable for a period of time is a concern.

akshatmittal (Reserve) commented:

@cccz - Let me clarify what @tbrent is trying to say.

Batch Auctions are always available, and the whole reason they exist is for scenarios like this. The key about reportViolation for Gnosis Trade is that it checks for an EasyAuction protocol invariant, and nothing else. That is why it’s the “backup” trading method.

Dutch Auctions and Batch Auctions have different characteristics from a what-happens-when perspective, but similar characteristics from a competition and pricing perspective. Yes, Dutch Auctions make it more efficient for the participants, which is why it exists, but it’s not something that the protocol entirely depends on.

Unavailability of Dutch Auction does not mean the protocol is at risk or is “unavailable”, which is why we disable Dutch Auctions in many more conditions than Batch Auctions. Keep in mind that early versions of the protocol did not even have Dutch Trade to begin with.

That said, we’ll leave it to you to decide here, we are internally considering it a low severity issue.

ether_sky (warden) commented:

Hi @akshatmittal - thanks for your comment.

I understood all your points. However, how can auditors know whether the Dutch Auctions is not important? The only thing which we could find is that there are only 2 types of auctions and one of them can be unavailable for some periods and each auctions can be paused due to reportViolation functionality.

Anyway, I will respect judge’s decision.

Thanks again.

Reserve mitigated

Status: Mitigation confirmed. Full details in reports from RadiantLabs, ether_sky and Bauchibred.

Low Risk and Non-Critical Issues

For this audit, 12 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by RadiantLabs received the top score from the judge.

The following wardens also submitted reports: Bauchibred, Shield, ether_sky, 0x52, SUPERMAN_I4G, Agontuk, PolarizedLight, Rhaydden, DanielArmstrong, krikolkk, and Aamir.

[L-01] Asymmetric decay of Asset saved priced distorts average price

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/plugins/assets/Asset.sol#L150-L163

Description

The Asset contract implements a graceful fallback for being able to provide underlying asset prices when the upstream oracle is unreachable.

This fallback uses the last known price, but artificially widens its range (low-high) to offset the uncertainty over the stale price.

The math of how this offset is done is implemented with this code;

File: Asset.sol
150:                 // Decay _high upwards to 3x savedHighPrice
151:                 // {UoA/tok} = {UoA/tok} * {1}
152:                 _high = savedHighPrice.safeMul(
153:                     FIX_ONE + MAX_HIGH_PRICE_BUFFER.muluDivu(delta - decayDelay, priceTimeout),
154:                     ROUND
155:                 ); // during overflow should not revert
156: 
157:                 // if _high is FIX_MAX, leave at UNPRICED
158:                 if (_high != FIX_MAX) {
159:                     // Decay _low downwards from savedLowPrice to 0
160:                     // {UoA/tok} = {UoA/tok} * {1}
161:                     _low = savedLowPrice.muluDivu(decayDelay + priceTimeout - delta, priceTimeout);
162:                     // during overflow should revert since a FIX_MAX _low breaks everything
163:                 }

We can see that when the decay starts (delta == decayDelay), the saved high/low readings are returned unchanged; then they start diverging linearly: at the end of the decay (left limit of delta == decayDelay + priceTimeout) low reaches 0 and high is scaled up by a multiplier of FIX_ONE + MAX_HIGH_PRICE_BUFFER, that is 3x its cached value.

Referring to this chart, because the two values - low (blue) and high (red) - diverge with a different slope, their average ((low + high) / 2, green) also varies over time and increases.

While this is not an issue for the Asset itself which doesn’t directly combine the two values with an average, it can be for downstream contracts that may do.

Consider changing MAX_HIGH_PRICE_BUFFER to FIX_ONE instead.

[L-02] “Flash” upgrade can be abused to create rigged but honest-looking RToken contracts

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/Main.sol#L111-L150

Description

The Main contract offers the possibility to upgrade Main and Component implementations, fetching the target addresses from an external versionRegistry provider.

Because the Deployer doesn’t set versionRegistry but let the contract admin provide it, and this entity can’t always be trusted as it’s specified in the permissionless Deployer.deploy function, the contract upgrade system can be abused to achieve a large variety of deviations from its intended behavior by:

• setting custom implementations
• tampering with the storage of all contracts (including balances and/or settings to bypass init sanity checks)
• restoring the contracts in a state that looks legitimate with proper implementations and governance

All of the above can be achieved in one single transaction (hence the “Flash upgrade” term used above), possibly to be bundled with the contract creation or buried in a long list of spammy interactions to lower the chances of detection; after a moderately sophisticated attack like the one presented in PoC there can be no signs of past wrongdoing in the result of any of the RToken contracts’ getters.

Consider having the Deployer set Main.versionRegistry at RToken deployment time to enforce continued use of trusted code.

[L-03] Prime basket weights are not properly validated

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L264

Description

When the Governance sets the prime basket compositions, for each of the provided collaterals, it is allowed to specify any target amount between 1 and MAX_TARGET_AMT (L264):

File: BasketHandler.sol
260:         for (uint256 i = 0; i < erc20s.length; ++i) {
261:             // This is a nice catch to have, but in general it is possible for
262:             // an ERC20 in the prime basket to have its asset unregistered.
263:             require(assetRegistry.toAsset(erc20s[i]).isCollateral(), "erc20 is not collateral");
264:             require(0 < targetAmts[i] && targetAmts[i] <= MAX_TARGET_AMT, "invalid target amount");
265: 
266:             config.erc20s.push(erc20s[i]);
267:             config.targetAmts[erc20s[i]] = targetAmts[i];
268:             names[i] = assetRegistry.toColl(erc20s[i]).targetName();
269:             config.targetNames[erc20s[i]] = names[i];
270:         }

This check is however insufficient because excessively low values of targetAmt, like 1, would likely cause overflows in the code that translates balances to assets / baskets like the BasketHandler.basketsHeldBy function.

Consider enforcing the range specified in the acceptable values for prime basket weights, at the very least by requiring targetAmts to be 1e-6 (D18) or more instead of the 1e-18 (D18) or more that is currently allowed.

[L-04] RToken issuance/redemption throttles can be monopolized by Bundlers and Batchers

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L121-L122
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L199-L200
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/RToken.sol#L277-L278

Description

When the RToken issuance and redemption are close to their limits, every upcoming issuance and redemption creates opportunity for a new operation in opposite direction to happen in the form of restored amount available for redemption or issuance respectively.

This means that actors that can control the ordering of transactions, and/or when a transaction is executed, like MEV bundlers and gasless transaction batchers, will have the upper hand on using these available amounts, potentially monopolizing them for themselves.

We don’t have a suggested mitigation for the contracts in the scope, but we’d rather issue a recommendation for users to use private mempools and avoid gasless transactions for operations involving RToken issuance and redemption.

[L-05] Broker accepts batchAuctionLength and dutchAuctionLength to be both 0

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/Broker.sol#L199
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/Broker.sol#L221

Description

When Broker is initialized, and later re-configured via governance calls, it validates the batchAuctionLength and dutchAuctionLength given parameters individually:

File: Broker.sol
073:     function init(
---
080:     ) external initializer {
---
101:         setBatchAuctionLength(batchAuctionLength_);
102:         setDutchAuctionLength(dutchAuctionLength_);
103:     }
---
197:     function setBatchAuctionLength(uint48 newAuctionLength) public governance {
198:         require(
199:             newAuctionLength == 0 ||
200:                 (newAuctionLength >= MIN_AUCTION_LENGTH && newAuctionLength <= MAX_AUCTION_LENGTH),
201:             "invalid batchAuctionLength"
202:         );
203:         emit BatchAuctionLengthSet(batchAuctionLength, newAuctionLength);
204:         batchAuctionLength = newAuctionLength;
205:     }
---
219:     function setDutchAuctionLength(uint48 newAuctionLength) public governance {
220:         require(
221:             newAuctionLength == 0 ||
222:                 (newAuctionLength >= MIN_AUCTION_LENGTH && newAuctionLength <= MAX_AUCTION_LENGTH),
223:             "invalid dutchAuctionLength"
224:         );
225:         emit DutchAuctionLengthSet(dutchAuctionLength, newAuctionLength);
226:         dutchAuctionLength = newAuctionLength;
227:     }

It does however not validate the situation when they are both provided as 0. This is a situation that is not admissible because in this case, no auction can be created and a BasketManager can’t open recollateralization trades.

Consider adding additional checks in setBatchAuctionLength and setDutchAuctionLength to prevent setting both to 0.

[L-06] PermitLib uses two different ERC1271 implementations for the same call

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/libraries/Permit.sol#L17

Description

PermitLib offers the requireSignature function, which calls isValidSignature if owner is a contract, and uses OZ’s isValidSignatureNow otherwise.

If we look at the isValidSignatureNow implementation of the imported OZ version, however, we can see that this, too, has a fallback call to IERC1271.isValidSignature.

The if isContract branch in requireSignature is therefore useless because it’s redundant with the other, more standard, branch and we therefore recommend removing it.

[L-07] VersionRegistry.latestVersion() does not support semantic versioning

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/registry/VersionRegistry.sol#L54

Description

From the version history of the Reserve protocol it appears that the protocol is using semantic versioning or a similar alternative.

This seems a use case that does not fit well with how VersionRegistry.latestVersion is updated: every time a new version is registered on VersionRegistry, latestVersion will point to that version.

In the event that the protocol releases 4.0.0 and shortly after 3.4.2, then latestVersion will incorrectly point to 3.4.2.

Consider adding a boolean flag to the registerVersion function, allowing the caller to specify whether latestVersion should be updated or not.

[L-08] DutchTrade.bidWithCallback() does not send back excess tokens

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/plugins/trading/DutchTrade.sol#L260

Description

DutchTrade.bidWithCallback allows for bidding with a callback hook that allows the bidder to swap the bought tokens for the sold tokens.

After the callback exits, the function checks that enough tokens are provided:

File: DutchTrade.sol
257:         uint256 balanceBefore = buy.balanceOf(address(this)); // {qBuyTok}
258:         IDutchTradeCallee(bidder).dutchTradeCallback(address(buy), amountIn, data);
259:         require(
260:             amountIn <= buy.balanceOf(address(this)) - balanceBefore,
261:             "insufficient buy tokens"
262:         );

However, if extra tokens are given, the function does not return the extras to the caller. Consider adding a check to return extra tokens if any are provided

[L-09] Missing check to avoid circular dependencies among RTokens

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/BasketHandler.sol#L166

Description

When BasketHandler switches basket to new reference amounts, it calls trackStatus():

File: BasketHandler.sol
158:     function refreshBasket() external {
159:         assetRegistry.refresh();
160: 
161:         require(
162:             main.hasRole(OWNER, _msgSender()) ||
163:                 (lastStatus == CollateralStatus.DISABLED && !main.tradingPausedOrFrozen()),
164:             "basket unrefreshable"
165:         );
166:         _switchBasket();
167: 
168:         trackStatus();
169:     }

This call sequence would not call the scenario when there is a circular dependency between RTokens, for example:

• RTokenA has RTokenB as collateral
• RTokenB governance is is unaware of their token being a collateral in RTokenA
• RTokenB adds RTokenA as collateral

At this point, price retrieval of either collateral would fail because of an infinite recursion.
While the RTokenB governance action can be seen as a mistake, RTokenA is affected too without any mistake made by its governance.

Consider adding a price() call after trackStatus() to trigger a failure in the above-mentioned case.

[L-10] StRSRVotes.delegateBySig() misses check for delegation to happen in the era intended by the signer

Links to affected code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/StRSRVotes.sol#L176

Description

The StRSRVotes.delegateBySig() function allows a signer to delegate voting power to a designed delegate via a gasless call.

If we look at the logic that verifies the signature verification:

File: StRSRVotes.sol
166:     function delegateBySig(
167:         address delegatee,
168:         uint256 nonce,
169:         uint256 expiry,
170:         uint8 v,
171:         bytes32 r,
172:         bytes32 s
173:     ) public {
174:         require(block.timestamp <= expiry, "signature expired");
175:         address signer = ECDSAUpgradeable.recover(
176:             _hashTypedDataV4(keccak256(abi.encode(_DELEGATE_TYPEHASH, delegatee, nonce, expiry))),
177:             v,
178:             r,
179:             s
180:         );
181:         require(nonce == _useDelegationNonce(signer), "invalid nonce");
182:         _delegate(signer, delegatee);
183:     }

We can see that the era is missing from the signed payload at L176.

This means that a delegation signature can be reused across era changes, despite balances and voting power no longer apply.

Consider adding an era field to the signed payload for StRSRVotes.delegateBySig()

[L-11] Potential DAO fee rounding to zero for upgraded RTokens

Upgraded RTokens might not pay DAO fees even if daoFeeRegistry has been set in Main. This is due to the change in the distribution total constraint in the _ensureSufficientTotal() function.

In previous versions, the check only required one of rTokenDist or rsrDist to be non-zero. The current implementation requires their sum to be greater than or equal to MAX_DISTRIBUTION. RTokens initialized with small distribution values in previous versions might not meet this new constraint after upgrading.

For small rTokenTotal and rsrTotal values, the DAO fee calculationin the distribute() function will round down to 0.

Consider implementing a post-upgrade call to _ensureSufficientTotal().

[L-12] Changing distributions between revenue split and distribution can affect DAO fee amounts

Building upon the issue identified in TRST-L-2 of the Trust Security audit report for v4.0.0, there are additional impacts related to changing distributions between when revenue is split in the BackingManager and when it’s distributed in the Distributor:

1. If RToken governance alters the distribution to heavily favor rsrTotal over rTokenTotal, they can significantly reduce the effective DAO fee for that cycle.
2. If the DAO changes the fee percentage during this period, the RToken might end up paying more or less than would be fair for that cycle. This is very likely to happen to one RToken or the other whenever veRSR governance changes the fee.

These issues, like the one identified in TRST-L-2, are temporary and limited to single distribution cycles. The first case could be repeated and even automated to continuously evade paying the appropriate DAO fee.

[L-13] Issuance premium calculation may be inaccurate for view functions

The issuancePremium() function in BasketHandler assumes that refresh() has been called on the collateral token in the current block. If this assumption is not met, it returns FIX_ONE instead of calculating the actual premium. While this is generally not an issue because the protocol holds this invariant, it can lead to inaccurate results when called from external view functions that cannot trigger a refresh(), such as RTokenAsset.price().

To improve accuracy and consistency, consider modifying issuancePremium() to calculate the premium regardless of when refresh() was last called. This would ensure more accurate premium values are returned even when accessed through view functions or external calls.

[L-14] Distribution validation is inconsistent due to inclusion of variable DAO fee

The Distributor._ensureSufficientTotal() function is called with the return values of totals(), which includes the DAO fee in its calculations. This can lead to a situation where previously valid distributions become invalid if the DAO fee is lowered, as the sum of rTokenTotal and rsrTotal may fall below MAX_DISTRIBUTION.

To ensure consistent behavior regardless of DAO fee changes, _ensureSufficientTotal() should only consider the sum of rTokenDist and rsrDist values from the actual distributions, excluding the DAO fee.

tbrent (Reserve) commented:

L-01 suggestion is great, honorable mention.

Mitigation Review

Introduction

Following the C4 audit, 4 wardens (RadiantLabs (3docSec and EV_om), ether_sky and Bauchibred) reviewed the mitigations for all identified issues. Additional details can be found within the C4 Reserve Core Mitigation Review repository.

Mitigation Review Scope

Mitigation of High & Medium Severity Issues

The sponsor requested some extra eyes on the following changes, since they are not in the “obviously safe” category:

Additional scope to be reviewed

The following items were additional changes that were in scope for this review, but not directly tied to findings from the original C4 audit.

Out of Scope

M-04: Dutch auctions can fail to settle if any other collateral in the basket behaves unexpectedly

Mitigation Review Summary

During the mitigation review, the wardens confirmed that all in-scope findings were mitigated. The table below provides details regarding the status of each in-scope vulnerability from the original audit.

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.