prePO contest - cccz'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, use globalWithdrawLimitPerPeriod and userWithdrawLimitPerPeriod to limit the number of tokens that can be withdrawn in a period.

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

However, there is no limit on the first withdrawal of a period, so that users can bypass the limit of globalWithdrawLimitPerPeriod and userWithdrawLimitPerPeriod.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

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

Lines of code

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

Vulnerability details

Impact

PrePOMarket.setFinalLongPayout is used to determine the final price of the LongToken

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

and after PrePOMarket.setFinalLongPayout is called, the user can call PrePOMarket.redeem to redeem the collateral, and the amount of collateral to be redeemed will be determined by the finalLongPayout.

  function redeem(uint256 _longAmount, uint256 _shortAmount) external override nonReentrant {
    require(longToken.balanceOf(msg.sender) >= _longAmount, "Insufficient long tokens");
    require(shortToken.balanceOf(msg.sender) >= _shortAmount, "Insufficient short tokens");

    uint256 _collateralAmount;
    if (finalLongPayout <= MAX_PAYOUT) {
      uint256 _shortPayout = MAX_PAYOUT - finalLongPayout;
      _collateralAmount = (finalLongPayout * _longAmount + _shortPayout * _shortAmount) / MAX_PAYOUT;

However, if PrePOMarket.setFinalLongPayout is called multiple times and different finalLongPayout is set, it will disrupt the user's redemption process.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

  function setFinalLongPayout(uint256 _finalLongPayout) external override onlyOwner {
+  require(finalLongPayout > MAX_PAYOUT, "Market ended");
    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-L83

Vulnerability details

Impact

MANAGER_WITHDRAW_ROLE can call Collateral.managerWithdraw to send baseToken to manager

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

setManager is called by SET_MANAGER_ROLE

  function setManager(address _newManager) external override onlyRole(SET_MANAGER_ROLE) {
    manager = _newManager;
    emit ManagerChange(_newManager);
  }

The number of baseTokens that MANAGER_WITHDRAW_ROLE can take out is determined by ManagerWithdrawHook.minReservePercentage

  function hook(address, uint256, uint256 _amountAfterFee) external view override { require(collateral.getReserve() - _amountAfterFee >= getMinReserve(), "reserve would fall below minimum"); }
...
  function getMinReserve() public view override returns (uint256) { return (depositRecord.getGlobalNetDepositAmount() * minReservePercentage) / PERCENT_DENOMINATOR; }

and setMinReservePercentage is called by SET_MIN_RESERVE_PERCENTAGE_ROLE

  function setMinReservePercentage(uint256 _newMinReservePercentage) external override onlyRole(SET_MIN_RESERVE_PERCENTAGE_ROLE) {
    require(_newMinReservePercentage <= PERCENT_DENOMINATOR, ">100%");
    minReservePercentage = _newMinReservePercentage;
    emit MinReservePercentageChange(_newMinReservePercentage);
  }

These roles can be directly granted by the owner, resulting in the owner being able to rug all baseToken in Collateral

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider using timelock