user's funds lock and incorrect code behavior because users withdrawal amount won't get reset for all users in each userPeriodLength in WithdrawHook contract

[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

according to the comments in code: "Every time userPeriodLength seconds passes, the amount withdrawn for all users will be reset to 0" (https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/interfaces/IWithdrawHook.sol#L64-L76). but in current implementation only one of the users userToAmountWithdrawnThisPeriod[] value gets reset and this will cause users to not be able to withdraw their tokens even if they follow the limitation. users which has high withdrawal amount only can withdraw when they reset the value of userPeriodLength and in each period only one user can do that because in each userPeriodLength only one user withdraw amounts gets reset. so over time in each period only one user could withdraw. for example if userPeriodLength was 1 day, in each day 1 user can withdraw and if protocol had 100K user some users would never withdraw in their life time.

Proof of Concept

This is hook() code in WithdrawHook:

  function hook(
    address _sender,
    uint256 _amountBeforeFee,
    uint256 _amountAfterFee
  ) external override onlyCollateral {
    require(withdrawalsAllowed, "withdrawals not allowed");
    if (lastGlobalPeriodReset + globalPeriodLength < block.timestamp) {
      lastGlobalPeriodReset = block.timestamp;
      globalAmountWithdrawnThisPeriod = _amountBeforeFee;
    } else {
      require(globalAmountWithdrawnThisPeriod + _amountBeforeFee <= globalWithdrawLimitPerPeriod, "global withdraw limit exceeded");
      globalAmountWithdrawnThisPeriod += _amountBeforeFee;
    }
    if (lastUserPeriodReset + userPeriodLength < block.timestamp) {
      lastUserPeriodReset = block.timestamp;
      userToAmountWithdrawnThisPeriod[_sender] = _amountBeforeFee;
    } else {
      require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");
      userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;
    }
    depositRecord.recordWithdrawal(_amountBeforeFee);
    uint256 _fee = _amountBeforeFee - _amountAfterFee;
    if (_fee > 0) {
      collateral.getBaseToken().transferFrom(address(collateral), _treasury, _fee);
      _tokenSender.send(_sender, _fee);
    }
  }

As you can see when the lastUserPeriodReset + userPeriodLength < block.timestamp happens(which means last period finished) only the callers userToAmountWithdrawnThisPeriod[] value changes and other don't get changed. and in the else case (when period is not finished) if user's userToAmountWithdrawnThisPeriod[user] + amount succeed userWithdrawLimitPerPeriod code would revert. so during periods users with high withdraw amount can't withdraw and in each period only one user withdraw amount gets new value so overtime all users get high withdraw amounts and in each period 1 user cant get his withdraws and users tokens would stuck in the contract literally forever. POC:

1. user1 withdraw 90 token and userToAmountWithdrawnThisPeriod[user1] would be 90. max allowed withdraw in each period for users is 100 token.
2. some time passes and we are still in current period, and user1 wants to withdraw 20 tokens but userToAmountWithdrawnThisPeriod[user1] is still 90 and 90+20 is bigger than 100 and user1 can't be able to withdraw.
3. this would happen to all users over time and only one user can withdraw in each period.

Tools Used

VIM

Recommended Mitigation Steps

reset all users withdrawal amounts in each period start time.

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

ramenforbreakfast marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[C4-Staff]

captainmangoC4 marked issue 116 as selected for report. Updating associated duplicate and primary issues.