prePO contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

In WithdrawHook.hook(), withdraw limits can be bypassed.

As a result, users might withdraw more amount of the base token at a time than they should.

Proof of Concept

WithdrawHook.hook() checks the withdraw limits like below.

    if (lastGlobalPeriodReset + globalPeriodLength < block.timestamp) { //@audit bypass limit
      lastGlobalPeriodReset = block.timestamp;
      globalAmountWithdrawnThisPeriod = _amountBeforeFee;
    } else {
      require(globalAmountWithdrawnThisPeriod + _amountBeforeFee <= globalWithdrawLimitPerPeriod, "global withdraw limit exceeded");
      globalAmountWithdrawnThisPeriod += _amountBeforeFee;
    }
    if (lastUserPeriodReset + userPeriodLength < block.timestamp) { //@audit bypass limit
      lastUserPeriodReset = block.timestamp;
      userToAmountWithdrawnThisPeriod[_sender] = _amountBeforeFee;
    } else {
      require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");
      userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;
    }

But it doesn't check the limit when it resets the period so users can withdraw without any limits at that time.

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify like below.

    if (lastGlobalPeriodReset + globalPeriodLength < block.timestamp) {
      lastGlobalPeriodReset = block.timestamp;
      globalAmountWithdrawnThisPeriod = _amountBeforeFee;
    } else {
      globalAmountWithdrawnThisPeriod += _amountBeforeFee;
    }
    require(globalAmountWithdrawnThisPeriod <= globalWithdrawLimitPerPeriod, "global withdraw limit exceeded");


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

Lines of code

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

Vulnerability details

Impact

If finalLongPayout is changed twice by admin fault, the market would be insolvent as it should pay more collateral than it has.

Proof of Concept

If finalLongPayout is less than MAX_PAYOUT, it means the market is ended and longToken Price = finalLongPayout, shortToken Price = MAX_PAYOUT - finalLongPayout.

So when users redeem their long/short tokens, the total amount of collateral tokens will be the same as the amount that users transferred during mint().

Btw in setFinalLongPayout(), there is no validation that this function can't be called twice and the below scenario would be possible.

1. Let's assume there is one user Bob in the market for simplicity.
2. Bob transferred 100 amounts of collateral and got 100 long/short tokens. The market has 100 collateral.
3. The market admin set finalLongPayout = 60 * 1e16 and Bob redeemed 100 longToken and received 60 collateral. The market has 40 collateral now.
4. After that, the admin realized finalLongPayout is too high and changed finalLongPayout = 40 * 1e16 again.
5. Bob tries to redeem 100 shortToken and receive 60 collateral but the market can't offer as it has 40 collateral only.

When there are several users in the market, some users can't redeem their long/short tokens as the market doesn't have enough collaterals.

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify setFinalLongPayout() like below so it can't be finalized twice.

  function setFinalLongPayout(uint256 _finalLongPayout) external override onlyOwner { 
    require(finalLongPayout > MAX_PAYOUT, "Finalized already"); //++++++++++++++++++++++++

    require(_finalLongPayout >= floorLongPayout, "Payout cannot be below floor");
    require(_finalLongPayout <= ceilingLongPayout, "Payout cannot exceed ceiling");
    finalLongPayout = _finalLongPayout;
    emit FinalLongPayoutSet(_finalLongPayout);
  }

Lines of code

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

Vulnerability details

Impact

This is an issue about admin privilege.

The admin can withdraw all base tokens using Collateral.managerWithdraw().

Proof of Concept

As we can see from here, the Collateral contract should keep a certain amount of base token for reserve.

There are several roles such as MANAGER_WITHDRAW_ROLE and SET_MANAGER_WITHDRAW_HOOK_ROLE and these roles can be set by DEFAULT_ADMIN_ROLE in SafeAccessControlEnumerableUpgradeable.sol.

So all base tokens might be withdrawn like below.

1. Don't update withdrawHook by SET_MANAGER_WITHDRAW_HOOK_ROLE so let withdrawHook = address(0)
2. When managerWithdraw() is called by MANAGER_WITHDRAW_ROLE, there is no validation at all so can withdraw all base tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend adding stick validation of reserve requirement or time lock for managerWithdraw().