Collateral can be easily stolen by hacked or malicious protocol owners

[code423n4]

Lines of code

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

Vulnerability details

The Collateral contract stores base tokens deposited as collateral in the protocol.

The contract contains a function managerWithdraw what would an account with the MANAGER_WITHDRAW_ROLE to withdraw any amount of funds from the contract:

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

A potentially attached ManagerWithdrawHook can minimize the size of the attack and the amount of withdrawn tokens, but it also relies on a minReservePercentage configurable variable by the protocol owners which can be set to 0 to nullify the minimum reserve amount that should be kept.

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 minReservePercentage can be set to 0 which will make getMinReserve() return 0.

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

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

Note that these roles are also assignable by the admin (DEFAULT_ADMIN_ROLE in OZ AccessControl contract) so any admin can also grant to themselves the required permissions to pull the attack.

Impact

Hacked or malicious manager or protocol owner can steal all base tokens from the collateral contracts.

Recommendation

There should be stronger immutable guarantees to limit how much a manager can withdraw and at what stage.

Judging Note

The status quo regarding significant centralization vectors has always been to award Medium severity, in order to warn users of the protocol of this category of risks. See here for list of centralization issues previously judged.

[Picodes]

Downgrading to QA following this thread: https://github.com/code-423n4/org/issues/59

This admin privilege is clearly per design.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-c