code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Checks for Global and User Withdraw Limit Per Period are missing for the first withdrawal request right AFTER period length expires and a new period begins. First withdrawal request amount after period length expires can be way higher than globalAmountWithdrawnThisPeriod.

Since timestamp when a next period begins is known well in advance, a large whale can time his withdrawal exactly when a period ends. By dumping a large size of $preCT tokens, a whale can create serious arbitrage opportunities viz-a-viz Uniswap V3 pool. Since this can have a significant short term disruption in supply, I've marked it as HIGH risk

In the WithdrawHook contract line 59, when block.timestamp > lastGlobalPeriodReset + globalPeriodLength, two variables lastGlobalPeriodReset and globalAmountWithdrawnThisPeriod are being reset.

However, for the first withdrawal request when the two above variables are being reset, check if _amountBeforeFee exceeds globalWithdrawLimitPerPeriod is missing. This check is done for the next request in line 63 when lastGlobalPeriodReset is set to previous block timestamp

Same problem also exists for lastUserPeriodReset and userToAmountWithdrawnThisPeriod[_sender] variables in

If a user makes a large withdrawal request right after a period expires, that request will be honored in existing codebase. This is in direct violation of the designed logic that puts restriction on global withdrawal and user specific withdrawal in each period

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Suppose global withdrawal limit per period if 10m USDC and each period is for 100 seconds. Let's say current timestamp is 10000 -> Bob, a whale depositor times a withdrawal request at exactly 10100 sec for 50m USDC -> Bob sources this liquidity of preCT tokens by dumping Long/Short tokens and draining preCT from UniV3 pools. This can cause massive mispricing of these tokens in short term

Although it can create massive arbitrage opportunities-> in current bear markets with low liquidity, such arbitrage might not be quickly closed. This can result in user panic & loss of confidence in protocol.

Tools Used

Recommended Mitigation Steps

Add the check when period gets reset. Added checks to following code block

     //global limit check
      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");
      globalAmountWithdrawnThisPeriod += _amountBeforeFee;
    }

    // user limit check
    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");
      userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;
    }

hansfriese commented 1 year ago

duplicate of #310

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #310