prePO contest - rvierdiiev's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L66-L68

Vulnerability details

Impact

Because WithdrawHook.hook doesn't track user's withdraw time separately, when someone withdraws before user, he can't withdraw all amount

Proof of Concept

Function WithdrawHook.hook should not allow users to withdraw more then is allowed for some period. https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L66-L72

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

This is logic controls how many tokens user can withdraw during some period. What you should note here is that there is no lastUserPeriodReset per account, it is used globally. And once any account withdraws when (lastUserPeriodReset + userPeriodLength < block.timestamp it will be reset to block.timestamp.

I want to show situation to explain the problem. Let's assume that allowed amount to withdraw per user is 100 tokens per hour. 1.At t1=0 lastUserPeriodReset is 0. userA calls withdraw with amount 100. userToAmountWithdrawnThisPeriod[_sender] becomes 100. And userA has no ability to withdraw more tokens during this hour. He will be able to do that at t2=1 hour and 1 minute. 2.At t2=1 hour and 1 minute userB calls withdraw 100 tokens. Because this condition if (lastUserPeriodReset + userPeriodLength < block.timestamp) is true, lastUserPeriodReset becomes block.timestamp and == 1 hour and 1 minute. What happened next to this userB is not interested for us. 3.At t3 = 1 hour and 5 minutes userA calls withdraw 100 tokens. Because this condition if (lastUserPeriodReset + userPeriodLength < block.timestamp) is false, this check will be done require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");. And it will fail, because userToAmountWithdrawnThisPeriod[_sender] is currently 100 and it was not cleared when userB updated lastUserPeriodReset. As result userA can't withdraw money currently. To withdraw them he should be first one who will call withdraw after lastUserPeriodReset + userPeriodLength time amount, as only in that case his userToAmountWithdrawnThisPeriod amount will not be checked.

Tools Used

VsCode

Recommended Mitigation Steps

You need to add one more bool field that will indicate that lastUserPeriodReset was recently reset.

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

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L59-L62 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L140-L146

Vulnerability details

Impact

WithdrawHook doesn't check that userWithdrawLimitPerPeriod is less than globalWithdrawLimitPerPeriod. It allows to user withdraw more then globalWithdrawLimitPerPeriod per period.

Proof of Concept

Function WithdrawHook.hook should not allow to withdraw more than globalWithdrawLimitPerPeriod amount of tokens per globalPeriodLength of time. But it's possible when userWithdrawLimitPerPeriod > globalWithdrawLimitPerPeriod. Because there is no input checkings it's possible to provide userWithdrawLimitPerPeriod > globalWithdrawLimitPerPeriod.

This is how this global withdraw limit is handled. https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L59-L65

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

As you can see the check is done only when globalPeriodLength has not passed since last lastGlobalPeriodReset update. When this condition if (lastGlobalPeriodReset + globalPeriodLength < block.timestamp) is true, then there is no amount validation. And if userWithdrawLimitPerPeriod is bigger than globalWithdrawLimitPerPeriod then user can withdraw more than globalWithdrawLimitPerPeriod. If userWithdrawLimitPerPeriod is less than globalWithdrawLimitPerPeriod then it will be not possible to do that, as user limit checking will fail(if they will be fixed as i described in another reports).

Tools Used

VsCode

Recommended Mitigation Steps

Add check that userWithdrawLimitPerPeriod < globalWithdrawLimitPerPeriod in setters. Or add checking require(_amountBeforeFee <= globalWithdrawLimitPerPeriod, "global withdraw limit exceeded"); to if clause.

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L17 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L29-L33 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L82

Vulnerability details

Impact

Collateral can become insolvent because of managerWithdraw function.

Proof of Concept

Collateral.managerWithdraw allows to withdraw base tokens to manager. https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L80-L83

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

In case if managerWithdrawHook is 0 then there is no any checks and _amount is sent to manager. This can make Collateral insolvent. Scenario. 1.User deposit 1 million tokens to Collateral. 2.managerWithdraw is called and all tokens were sent to manager. 3.Collateral is insolvent, users can't withdraw.

In case if managerWithdrawHook is not 0 then managerWithdrawHook.hook is called. https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L17

  function hook(address, uint256, uint256 _amountAfterFee) external view override { require(collateral.getReserve() - _amountAfterFee >= getMinReserve(), "reserve would fall below minimum"); }

The check that is done here is that balance of base token of Collateral will stay above some provided percentage(minReservePercentage) multiplied by all amount recorded in depositRecord after withdraw. https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L41

  function getMinReserve() public view override returns (uint256) { return (depositRecord.getGlobalNetDepositAmount() * minReservePercentage) / PERCENT_DENOMINATOR; }

Even if this percentage is small, it's enough to make Collateral insolvent as well and Collateral will not hold enough funds to cover users withdraws.

Tools Used

VsCode

Recommended Mitigation Steps

I believe that manager should not be able to withdraw users funds. He should not be able to withdraw any amount that will make Collateral balance less than depositRecord.getGlobalNetDepositAmount().

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/DepositTradeHelper.sol#L24

Vulnerability details

Impact

DepositTradeHelper.depositAndTrade will not work normally with ERC20 that need to clear allowance as it sets allowance to uint256.max and do not set it to 0 after. As result next time when user calls depositAndTrade function it will fail as allowance should be set to 0 before.

Proof of Concept

DepositTradeHelper.depositAndTrade function allows user to deposit base tokens into Collateral token and then swap it using Uniswap. https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/DepositTradeHelper.sol#L22-L34

  function depositAndTrade(uint256 baseTokenAmount, Permit calldata baseTokenPermit, Permit calldata collateralPermit, OffChainTradeParams calldata tradeParams) external override {
    if (baseTokenPermit.deadline != 0) {
      IERC20Permit(address(_baseToken)).permit(msg.sender, address(this), type(uint256).max, baseTokenPermit.deadline, baseTokenPermit.v, baseTokenPermit.r, baseTokenPermit.s);
    }
    _baseToken.transferFrom(msg.sender, address(this), baseTokenAmount);
    if (collateralPermit.deadline != 0) {
      _collateral.permit(msg.sender, address(this), type(uint256).max, collateralPermit.deadline, collateralPermit.v, collateralPermit.r, collateralPermit.s);
    }
    uint256 _collateralAmountMinted = _collateral.deposit(msg.sender, baseTokenAmount);
    _collateral.transferFrom(msg.sender, address(this), _collateralAmountMinted);
    ISwapRouter.ExactInputSingleParams memory exactInputSingleParams = ISwapRouter.ExactInputSingleParams(address(_collateral), tradeParams.tokenOut, POOL_FEE_TIER, msg.sender, tradeParams.deadline, _collateralAmountMinted, tradeParams.amountOutMinimum, tradeParams.sqrtPriceLimitX96);
    _swapRouter.exactInputSingle(exactInputSingleParams);
  }

It uses permit function to provide allowance to himself and it sets allowance to type(uint256).max. After the transfer it doesn't set allowance to 0. And next time when user will call this function it will revert if _baseToken doesn't support providing allowance when current allowance is not set to 0(like USDT).

Tools Used

VsCode

Recommended Mitigation Steps

Do not set allowance to type(uint256).max, set it to baseTokenAmount value.