prePO contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

according to the comments in code: "Every time userPeriodLength seconds passes, the amount withdrawn for all users will be reset to 0" (https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/interfaces/IWithdrawHook.sol#L64-L76). but in current implementation only one of the users userToAmountWithdrawnThisPeriod[] value gets reset and this will cause users to not be able to withdraw their tokens even if they follow the limitation. users which has high withdrawal amount only can withdraw when they reset the value of userPeriodLength and in each period only one user can do that because in each userPeriodLength only one user withdraw amounts gets reset. so over time in each period only one user could withdraw. for example if userPeriodLength was 1 day, in each day 1 user can withdraw and if protocol had 100K user some users would never withdraw in their life time.

Proof of Concept

This is hook() code in WithdrawHook:

  function hook(
    address _sender,
    uint256 _amountBeforeFee,
    uint256 _amountAfterFee
  ) external override onlyCollateral {
    require(withdrawalsAllowed, "withdrawals not allowed");
    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;
    }
    depositRecord.recordWithdrawal(_amountBeforeFee);
    uint256 _fee = _amountBeforeFee - _amountAfterFee;
    if (_fee > 0) {
      collateral.getBaseToken().transferFrom(address(collateral), _treasury, _fee);
      _tokenSender.send(_sender, _fee);
    }
  }

As you can see when the lastUserPeriodReset + userPeriodLength < block.timestamp happens(which means last period finished) only the callers userToAmountWithdrawnThisPeriod[] value changes and other don't get changed. and in the else case (when period is not finished) if user's userToAmountWithdrawnThisPeriod[user] + amount succeed userWithdrawLimitPerPeriod code would revert. so during periods users with high withdraw amount can't withdraw and in each period only one user withdraw amount gets new value so overtime all users get high withdraw amounts and in each period 1 user cant get his withdraws and users tokens would stuck in the contract literally forever. POC:

1. user1 withdraw 90 token and userToAmountWithdrawnThisPeriod[user1] would be 90. max allowed withdraw in each period for users is 100 token.
2. some time passes and we are still in current period, and user1 wants to withdraw 20 tokens but userToAmountWithdrawnThisPeriod[user1] is still 90 and 90+20 is bigger than 100 and user1 can't be able to withdraw.
3. this would happen to all users over time and only one user can withdraw in each period.

Tools Used

VIM

Recommended Mitigation Steps

reset all users withdrawal amounts in each period start time.

Lines of code

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

Vulnerability details

Impact

Function setFinalLongPayout() Sets the payout a Long token(finalLongPayout) can be redeemed for after the market has ended. function redeem() calculates the redeem amount based on the finalLongPayout and if the value of finalLongPayout changes then users would receive other redeem amounts(profits). owner most be able to set the value of the finalLongPayout one time because if owner set multiple values for finalLongPayout users would get different profits for their positions and some users would lose their profits. as the final value of the asset is one number in the real world so owner should only be able to set one value for finalLongPayout one time.

Proof of Concept

This is setFinalLongPayout() code:

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

As you can see there is no restriction on how many times owner can set this value and the profit and loss calculation in the redeem() function for long or short positions has been calculated based on finalLongPayout value. so if this scenario happens:

1. owner set high value for finalLongPayout.
2. some user redeem their positions and long users would profit a lot.
3. owner change the value of finalLongPayout and set low value.
4. other long position user would lose their funds instead of profiting.

so users get different profit and loss if owner makes this mistake and contract shouldn't let owner to perform this action because when market is finished and ended(when owner calls this function it means market has ended) owner shouldn't be able to set new finalLongPayout.

Tools Used

VIM

Recommended Mitigation Steps

prevent owner from changing the value of finalLongPayout after market is closed.