prePO contest - Trust's results

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

Lines of code

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

Vulnerability details

Description

In PrePOMarket.sol, the _finalLongPayout represents the finalized value of a single long token in the market. It is settled post ICO / IPO according to predetermined rules. The issue is that this value may be re-initialized as many times as owner desires:

function setFinalLongPayout(uint256 _finalLongPayout) external override onlyOwner {
  require(_finalLongPayout >= floorLongPayout, "Payout cannot be below floor");
  require(_finalLongPayout <= ceilingLongPayout, "Payout cannot exceed ceiling");
  finalLongPayout = _finalLongPayout;
  emit FinalLongPayoutSet(_finalLongPayout);
}

This introduces a very substantial centralization risk. A malicious owner may set long payout to floorLongPayout and cash out on all their short tokens, which would be worth their maximum potential. Then, they could re-initialize it to ceilingLongPayout and cash out their long tokens. This way, they are directly stealing from long / short token holders which will not be able to claim their underlying holdings.

For reference, this block in redeem() calculates the collateral amount. It clearly shows how the long tokens appreciate in value when finalLongPayout is high, and short tokens appreciate in value when finalLongPayout is low.

uint256 _collateralAmount;
if (finalLongPayout <= MAX_PAYOUT) {
  uint256 _shortPayout = MAX_PAYOUT - finalLongPayout;
  _collateralAmount = (finalLongPayout * _longAmount + _shortPayout * _shortAmount) / MAX_PAYOUT;
} else {
  require(_longAmount == _shortAmount, "Long and Short must be equal");
  _collateralAmount = _longAmount;
}

M - finalLongPayout can be reinitialized many times although by design it is final. Add require. Risk of owner misusing to enjoy high evaluation of Long position and then high evaluation of short position.

Impact

Owner can rug PrePOMarket using re-initialized finalLongPayout.

Tools Used

Manual audit

Recommended Mitigation Steps

finalLongPayout should only be allowed to be set once.

Judge has assessed an item in Issue #315 as M risk. The relevant finding follows:

Lines of code https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L29

Vulnerability details Description Collateral.sol exposes a permissioned withdraw function:

function managerWithdraw(uint256 _amount) external override onlyRole(MANAGER_WITHDRAW_ROLE) nonReentrant { if (address(managerWithdrawHook) != address(0)) managerWithdrawHook.hook(msg.sender, _amount, _amount); baseToken.transfer(manager, _amount); } An approved manager can withdraw the desired amount provided the hook executes successfully.

The hooks implementation is:

function hook(address, uint256, uint256 _amountAfterFee) external view override { require(collateral.getReserve() - _amountAfterFee >= getMinReserve(), "reserve would fall below minimum"); } The code above checks the remaining collateral amount is above a calculated minimum reserve. getMinReserve():

function getMinReserve() public view override returns (uint256) { return (depositRecord.getGlobalNetDepositAmount() * minReservePercentage) / PERCENT_DENOMINATOR; } It returns the current total deposited amount multiplied by minReservePercentage. The issue is that owner can immediately change minReservePercentage to zero:

function setMinReservePercentage(uint256 _newMinReservePercentage) external override onlyRole(SET_MIN_RESERVE_PERCENTAGE_ROLE) { require(_newMinReservePercentage <= PERCENT_DENOMINATOR, ">100%"); minReservePercentage = _newMinReservePercentage; emit MinReservePercentageChange(_newMinReservePercentage); } The combination of setting minimum percentage to zero, and allowing withdraw if leaving at least minimum percentage, allows owner to instantly steal the entire reserve held in Collateral.sol.

Impact Compromised manager + Withdraw hook manager can steal entire Collateral.sol reserves.

Tools Used Manual audit

Recommended Mitigation Steps Don't allow minReservePercentage to drop below a constant high number.

Lines of code

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

Vulnerability details

Description

In collateral deposit() and withdraw() flow, a fee is calculated as a percentage of user's requested amount. It is passed to the DepositHook and WithdrawHook, for example in deposit():

uint256 _amountAfterFee = _amount - _fee;
if (address(depositHook) != address(0)) {
  baseToken.approve(address(depositHook), _fee);
  // hook will calculate fee = _amount - _amountAfterFee
  depositHook.hook(_recipient, _amount, _amountAfterFee);
  baseToken.approve(address(depositHook), 0);
}

In the DepositHook, if there is a fee two actions take place:

1. Collect the fee to the treasury from Collateral contract
2. Send rewards relative to fee amount, to the depositor

uint256 _fee = _amountBeforeFee - _amountAfterFee;
if (_fee > 0) {
  collateral.getBaseToken().transferFrom(address(collateral), _treasury, _fee);
  _tokenSender.send(_sender, _fee);
}

The issue is that TokenSender has an early exit problem:

function send(address recipient, uint256 unconvertedAmount) external override onlyAllowedMsgSenders {
  uint256 scaledPrice = (_price.get() * _priceMultiplier) / MULTIPLIER_DENOMINATOR;
  if (scaledPrice <= _scaledPriceLowerBound) return;
  uint256 outputAmount = (unconvertedAmount * _outputTokenDecimalsFactor) / scaledPrice;
  if (outputAmount == 0) return;
  // EARLY EXIT PROBLEM
  if (outputAmount > _outputToken.balanceOf(address(this))) return;
  _outputToken.transfer(recipient, outputAmount);
}

In the code above, note that if the TokenSender does not currently have enough reward tokens to hand out, it will simply return successfully from the call. Therefore, user which assumed they will be getting cash back rewards from fees when depositing or withdrawing, are actually paying the fees with no compensation.

Impact

Permanent freeze of yield when TokenSender rewards bank is depleted and deposit or withdraw is called.

Proof of Concept

1. User has $1000 USDC
2. User deposits it to Collateral.sol, which charges 10% deposit fee
3. Meanwhile, TokenSender offers PPO rewards, but has just ran out.
4. DepositHook calls TokenSender.send(), which should send $100 fee rewards in PPO to user. send() returns without paying.
5. User is unhappy having assumed they will receive PPO from the fee.

Tools Used

Manual audit

Recommended Mitigation Steps

The root cause is that there is no differentiation between user's request to mint expecting rewards, and user's request to mint in any case. This lack of acknoledgement means user may be left under-compensated.

It is very advisable to add "forfeitRewards" flag, which is required to be true when TokenSender is not able to satisfy the reward owings to user.

Judging note

Permanent freezing of unclaimed yield is always rated as High severity on Immunefi bounties.