A whale user is able to cause freeze of funds of other users by bypassing withdraw limit

[code423n4]

Lines of code

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

Vulnerability details

Description

In Collateral.sol, users may withdraw underlying tokens using withdraw. Importantly, the withdrawal must be approved by withdrawHook if set:

function withdraw(uint256 _amount) external override nonReentrant {
  uint256 _baseTokenAmount = (_amount * baseTokenDenominator) / 1e18;
  uint256 _fee = (_baseTokenAmount * withdrawFee) / FEE_DENOMINATOR;
  if (withdrawFee > 0) { require(_fee > 0, "fee = 0"); }
  else { require(_baseTokenAmount > 0, "amount = 0"); }
  _burn(msg.sender, _amount);
  uint256 _baseTokenAmountAfterFee = _baseTokenAmount - _fee;
  if (address(withdrawHook) != address(0)) {
    baseToken.approve(address(withdrawHook), _fee);
    withdrawHook.hook(msg.sender, _baseTokenAmount, _baseTokenAmountAfterFee);
    baseToken.approve(address(withdrawHook), 0);
  }
  baseToken.transfer(msg.sender, _baseTokenAmountAfterFee);
  emit Withdraw(msg.sender, _baseTokenAmountAfterFee, _fee);
}

The hook requires that two checks are passed:

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;
}

If it has been less than "globalPeriodLength" seconds since the global reset, we step into the if block, reset time becomes now and starting amount is the current requested amount. Otherwise, the new amount must not overpass the globalWithdrawLimitPerPeriod. Very similar check is done for "user" variables.

The big issue here is that the limit can be easily bypassed by the first person calling withdraw in each group ("global" and "user"). It will step directly into the if block where no check is done, and fill the variable with any input amount.

As I understand, the withdraw limit is meant to make sure everyone is guaranteed to be able to withdraw the specified amount, so there is no chance of freeze of funds. However, due to the bypassing of this check, a whale user is able to empty the current reserves put in place and cause a freeze of funds for other users, until the Collateral contract is replenished.

Impact

A whale user is able to cause freeze of funds of other users by bypassing withdraw limit

Proof of Concept

1. Collateral.sol has 10000 USDC reserve
2. Withdraw limit is 150 USDC per user per period
3. There are 5 users - Alpha with collateral worth 12,000 USDC, and 4 users each with 1,000 USDC
4. Alpha waits for a time when request would create a new lastGlobalPeriodReset and new lastUserPeriodReset. He requests a withdraw of 10,000 USDC
5. The hook is passed and he withdraws the entire collateral reserves
6. At this point, victim Vic is not able to withdraw their 150 USDC. It is a freeze of funds.

Tools Used

Manual audit

Recommended Mitigation Steps

Add limit checks in the if blocks as well, to make sure the first request does not overflow the limit.

Judge note

I've confirmed with PrePO team during the contest that withdraw limit bypass is a very serious issue

[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-judge]

Picodes marked the issue as selected for report