prePO contest - aviggiano's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L66-L69 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L59-L62

Vulnerability details

Impact

Withdraw hook period limits (userWithdrawLimitPerPeriod and globalWithdrawLimitPerPeriod) are not enforced when the period is reset. Because of this, users can withdraw more than the period limits.

Proof of Concept

1. Alice deposits the base token on the Collateral contract
2. Alice waits some time, until the check lastUserPeriodReset + userPeriodLength < block.timestamp passes
3. Alice withdraws the full amount of the base token from the Collateral

Tools Used

Manual inspection

Recommended Mitigation Steps

Check for period limits on period reset

diff --git a/apps/smart-contracts/core/contracts/WithdrawHook.sol b/apps/smart-contracts/core/contracts/WithdrawHook.sol
index 546b2b5..b693f2b 100644
--- a/apps/smart-contracts/core/contracts/WithdrawHook.sol
+++ b/apps/smart-contracts/core/contracts/WithdrawHook.sol
@@ -58,6 +58,7 @@ contract WithdrawHook is IWithdrawHook, TokenSenderCaller, SafeAccessControlEnum
     require(withdrawalsAllowed, "withdrawals not allowed");
     if (lastGlobalPeriodReset + globalPeriodLength < block.timestamp) {
       lastGlobalPeriodReset = block.timestamp;
+      require(_amountBeforeFee <= globalWithdrawLimitPerPeriod, "global withdraw limit exceeded");
       globalAmountWithdrawnThisPeriod = _amountBeforeFee;
     } else {
       require(globalAmountWithdrawnThisPeriod + _amountBeforeFee <= globalWithdrawLimitPerPeriod, "global withdraw limit exceeded");
@@ -65,6 +66,7 @@ contract WithdrawHook is IWithdrawHook, TokenSenderCaller, SafeAccessControlEnum
     }
     if (lastUserPeriodReset + userPeriodLength < block.timestamp) {
       lastUserPeriodReset = block.timestamp;
+      require(_amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");
       userToAmountWithdrawnThisPeriod[_sender] = _amountBeforeFee;
     } else {
       require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");