prePO contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

User can bypass the userWithdrawLimitPerPeriod check by transferring balance to another account

Proof of Concept

1. Assume userWithdrawLimitPerPeriod is set to 1000
2. User A has current deposit of amount 2000 and wants to withdraw everything instantly
3. User A calls the withdraw function and takes out the 1000 amount

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

4. Remaining 1000 amount cannot be withdrawn since userWithdrawLimitPerPeriod is reached

function hook(
    address _sender,
    uint256 _amountBeforeFee,
    uint256 _amountAfterFee
  ) external override onlyCollateral {
...
require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");
...
}

5. User simply transfer his balance to his another account and withdraw from that account

6. Since withdraw limit is tied to account so this new account will be allowed to make withdrawal thus bypassing userWithdrawLimitPerPeriod

Recommended Mitigation Steps

User should only be allowed to transfer leftover limit. For example if User already utilized limit X then he should only be able to transfer userWithdrawLimitPerPeriod-X

Lines of code

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

Vulnerability details

Impact

It seems MANAGER_WITHDRAW_ROLE can steal entire user balance if SET_MIN_RESERVE_PERCENTAGE_ROLE has set setMinReservePercentage to 0%

Proof of Concept

1. The SET_MIN_RESERVE_PERCENTAGE_ROLE role sets minReservePercentage to 0%

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

2. MANAGER_WITHDRAW_ROLE role simply calls the managerWithdraw with contract balance as _amount

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

3. This calls hook which allow to withdraw everything above reserve

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

4. getMinReserve is implemented as below. Since minReservePercentage is 0 so getMinReserve becomes 0

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

5. This means user will be able to withdraw full collateral.getReserve() - _amountAfterFee which is approx entire balance (without fees)

function getReserve() external view override returns (uint256) { return baseToken.balanceOf(address(this)); }

Recommended Mitigation Steps

Do not allow SET_MIN_RESERVE_PERCENTAGE_ROLE to set setMinReservePercentage to 0

Use safeTransferFrom instead of transferFrom

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

Issue: While depositing contract is making use of transferFrom function which is insecure and the function fails to check the outcome of the transferFrom function (whether it was success or not)

function deposit(address _recipient, uint256 _amount) external override nonReentrant returns (uint256) {
    uint256 _fee = (_amount * depositFee) / FEE_DENOMINATOR;
    if (depositFee > 0) { require(_fee > 0, "fee = 0"); }
    else { require(_amount > 0, "amount = 0"); }
    baseToken.transferFrom(msg.sender, address(this), _amount);
...
}

Recommendation: Use safeTransferFrom which checks the outcome of the transfer and fails if transfer was not success