prePO contest - wait's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/9492c4d2641a9d395b1ca26d91716680c7640e25/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L52-L60

Vulnerability details

Impact

The minimum reserve can be set to zero (in fact it is zero default), putting fund at significant risk

Proof of Concept

Function ManagerWithdrawHook.sol#getMinReserve is used to limit how much reserve the manager can withdraw:

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

From the above code we can see that minReservePercentage determines the minimum reserve percentage. However, minReservePercentage is left unset by default (nothing is done in constructor), meaning it defaults to 0. Also, ManagerWithdrawHook.sol#setMinReservePercentage has no constraint on its minimum value, meaning it can be set to 0 manually.

This is a big risk for the reserves because manager can withdraw all the reserves at any time. i.e. all user funds can be drained easily.

Tools Used

Manual

Recommended Mitigation Steps

1. Add a constant as a lower bound of minReservePercentage :

uint256 public constant MIN_RESERVE_LOW_BOUND = 500000;

2. Add a constructor to ManagerWithdrawHook.sol:

constructor() {
  minReservePercentage = MIN_RESERVE_LOW_BOUND;
}

3. Add additional checking to ManagerWithdrawHook.sol#setMinReservePercentage

require(_newMinReservePercentage >= MIN_RESERVE_LOW_BOUND);

Lines of code

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

Vulnerability details

Impact

All deposited user assets may be drained.

Proof of Concept

Deposited user assets can be withdrawn by Collateral.sol#managerWithdraw:

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

All deposited assets can be drained easily and quickly if admin account(ADMIN_ROLE) is leaked or manager accounts(MANAGER_WITHDRAW_ROLE, SET_MANAGER_ROLE) are leaked.

All the hacker has to do is set the manager(to his EOA) and ManagerWithdrawHook (to 0), and then invoke managerWithdrawHook to transfer all base tokens stored in the contract to his account.

Tools Used

Manual

Recommended Mitigation Steps

We should make manager and managerWithdrawHook immutable, their addresses are set to deployed open source contracts when the contract is created so that no one can change them.