prePO contest - HE1M's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L29

Vulnerability details

Impact

Having separate roles for each critical task, is a good security action. But, the number of roles define the level of security. In this protocol, if three roles are malicious, they can drain the protocol.

Proof of Concept

The three roles SET_MANAGER_ROLE, MANAGER_WITHDRAW_ROLE, and SET_MIN_RESERVE_PERCENTAGE_ROLE can drain all the fund in the collateral reserve, if they are compromised or act maliciously.

The flow is:

• The malicious SET_MANAGER_ROLE changes the manager address.

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

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

• The malicious SET_MIN_RESERVE_PERCENTAGE_ROLE, sets the minReservePercentage equal to zero.

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

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L29

• The malicious MANAGER_WITHDRAW_ROLE withdraws all fund from collateral reserve to the new malicious manager. Since, minReservePercentage was set to zero in step two, getMinReserve() will return zero. So, the condition require(collateral.getReserve() - _amountAfterFee >= getMinReserve(), "reserve would fall below minimum"); allow to drain all the reserve.

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

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

Tools Used

Recommended Mitigation Steps

It is recommended to define another role who sets the limit for the minimum value of minReservePercentage .

function setMinReservePercentage(uint256 _newMinReservePercentage) external override onlyRole(SET_MIN_RESERVE_PERCENTAGE_ROLE) {
    require(_newMinReservePercentage <= PERCENT_DENOMINATOR, ">100%");
    require((_newMinReservePercentage * 100)/PERCENT_DENOMINATOR >= minReserveLimit, "withdrawing more than allowed");
    minReservePercentage = _newMinReservePercentage;
    emit MinReservePercentageChange(_newMinReservePercentage);
  }
function setMinReserveLimit(uint256 _newMinReserveLimit) external override onlyRole(SET_MIN_RESERVE_LIMIT_ROLE) {
    minReserveLimit = _newMinReserveLimit;
    emit MinReserveLimitChange(_newMinReserveLimit);
  }

Or add the condition require((_newMinReservePercentage * 100)/PERCENT_DENOMINATOR >= 50, "withdrawing more than 50%"); to the function setMinReservePercentage, to not allow withdrawing more than 50%.