The default Governor Anastasius is unable to call `resetStakes`

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSR.sol#L490

Vulnerability details

Description

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");
    }

Assessed type

Error

[akshatmittal]

Confirmed.

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report