Per-user withdraw limit not handled correctly

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/WithdrawHook.sol#L53-L79

Vulnerability details

Impact

WithdrawHook.sol implements a per-user withdraw limit that resets over time. But the contract logic only resets the user limit for the first user to withdraw after the userPeriodLength, all the other users before the next lastUserPeriodReset will keep their respective limits unchanged.

When userPeriodLength interval has passed, the next user call to withdraw (consider globalWithdrawLimitPerPeriod has not been reached) will go into the first branch of the if-else-statement (See code below), set lastUserPeriodReset to block.timestamp and reset this user limits. Further withdraws before the next lastUserPeriodReset will go into the second branch of the if-else-statement, which adds to the users' limits without reseting them. Therefore all users with the exception of the first will still have outdated limits even though their limits should have been reset.

Over time this can lead to many users reaching max limit and being unable to withdraw until they are the first one to withdraw after userPeriodLength has passed. A malicious actor could also keep frontrunning those first calls to withdraw, effectively blocking any user to reset their limit.

if (lastUserPeriodReset + userPeriodLength < block.timestamp) {
    lastUserPeriodReset = block.timestamp;
    userToAmountWithdrawnThisPeriod[_sender] = _amountBeforeFee;
} else {
    require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");
    userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;
}

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/WithdrawHook.sol#L66-L72

Proof of Concept

Steps to reproduce.

1. First user withdraws max value allowed by user limit
2. Second user withdraws max value allowed by user limit
3. Wait until userPeriodLength has passed
4. First user tries to withdraw, his limit resets and withdraw succeeds
5. Second user tries to withdraw, but he still have his old limit maxed out, therefore withdraw reverts.

diff --git a/WithdrawHook.test.ts.orig b/WithdrawHook.test.ts
index 234567b..5e5841b 100644
--- a/WithdrawHook.test.ts.orig
+++ b/WithdrawHook.test.ts
@@ -22,6 +22,7 @@ describe('=> WithdrawHook', () => {
   let withdrawHook: WithdrawHook
   let deployer: SignerWithAddress
   let user: SignerWithAddress
+  let user2: SignerWithAddress
   let collateral: FakeContract<Contract>
   let collateralSigner: SignerWithAddress
   let depositRecord: MockContract<Contract>
@@ -34,11 +35,11 @@ describe('=> WithdrawHook', () => {
   const TEST_AMOUNT_AFTER_FEE = parseEther('1')
   const TEST_GLOBAL_PERIOD_LENGTH = 20
   const TEST_USER_PERIOD_LENGTH = 10
-  const TEST_GLOBAL_WITHDRAW_LIMIT = TEST_AMOUNT_BEFORE_FEE.mul(3)
+  const TEST_GLOBAL_WITHDRAW_LIMIT = TEST_AMOUNT_BEFORE_FEE.mul(4)
   const TEST_USER_WITHDRAW_LIMIT = TEST_AMOUNT_BEFORE_FEE.mul(2)

   beforeEach(async () => {
-    ;[deployer, user, treasury] = await ethers.getSigners()
+    ;[deployer, user, user2, treasury] = await ethers.getSigners()
     depositRecord = await smockDepositRecordFixture(
       TEST_GLOBAL_DEPOSIT_CAP,
       TEST_ACCOUNT_DEPOSIT_CAP
@@ -556,6 +557,35 @@ describe('=> WithdrawHook', () => {
           withdrawHook.connect(collateralSigner).hook(user.address, 1, 1)
         ).to.revertedWith('user withdraw limit exceeded')
       })
+
+      it('POC', async () => {
+        //max limit for first user
+        await withdrawHook
+          .connect(collateralSigner)
+          .hook(user.address, TEST_USER_WITHDRAW_LIMIT, TEST_USER_WITHDRAW_LIMIT)
+
+        //max limit for second user
+        await withdrawHook
+          .connect(collateralSigner)
+          .hook(user2.address, TEST_USER_WITHDRAW_LIMIT, TEST_USER_WITHDRAW_LIMIT)
+
+        //
+        const previousResetTimestamp = await getLastTimestamp(ethers.provider)
+        await setNextTimestamp(
+          ethers.provider,
+          previousResetTimestamp + TEST_GLOBAL_PERIOD_LENGTH + 1
+        )
+
+        //first user withdraw, works as expected.
+        await withdrawHook
+          .connect(collateralSigner)
+          .hook(user.address, 1, 1)
+
+        //second user tries to withdraw, but reverts due to his limit not being updated correctly.
+        await expect(
+          withdrawHook.connect(collateralSigner).hook(user2.address, 1, 1)
+        ).to.revertedWith('user withdraw limit exceeded')
+      })
     })
   })

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/test/WithdrawHook.test.ts

• TEST_GLOBAL_WITHDRAW_LIMIT had to be changed to allow two users maxing out their limits.

[c4-judge]

Picodes marked the issue as duplicate of #325

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as not selected for report

[c4-judge]

Picodes marked the issue as satisfactory