WithdrawHook#userWithdrawLimitPerPeriod Limit max withdraw logic error

[code423n4]

Lines of code

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

Vulnerability details

Impact

The maximum withdrawal value per Period for the user may be much smaller than userWithdrawLimitPerPeriod

Proof of Concept

WithdrawHook#hook() will limit the user's maximum withdrawal per Period. The logic is: when a new Period , the new Period starts counting from 0.
But there is a logic error that causes this: only the first user of the Period is recalculated, the other users are not recalculated, and the new Period follows the userToAmountWithdrawnThisPeriod[_sender] of the previous cycle

lastUserPeriodReset needs to be changed to mapping(address => uint256), independent for each user

  function hook(
    address _sender,
    uint256 _amountBeforeFee,
    uint256 _amountAfterFee
  ) external override onlyCollateral {
..

    if (lastUserPeriodReset + userPeriodLength < block.timestamp) {
      lastUserPeriodReset = block.timestamp; //***@audit All users share a common “lastUserPeriodReset” ***//
      userToAmountWithdrawnThisPeriod[_sender] = _amountBeforeFee;  //**@audit only first user of period,  counting from 0***//
    } else {
      require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");
      userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;
    }

  }

Tools Used

Recommended Mitigation Steps

lastUserPeriodReset needs to be changed to mapping(address => uint256), independent for each user

contract WithdrawHook is IWithdrawHook, TokenSenderCaller, SafeAccessControlEnumerable {
..
- uint256 private lastUserPeriodReset;
+ mapping(address => uint256) private lastUserPeriodReset;

...

  function hook(
    address _sender,
    uint256 _amountBeforeFee,
    uint256 _amountAfterFee
  ) external override onlyCollateral {
...

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

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

...

[hansfriese]

duplicate of #310

[c4-judge]

Picodes marked the issue as duplicate of #310

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)