Compromised manager + Withdraw hook manager can steal entire Collateral.sol reserves.

[code423n4]

Lines of code

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

Vulnerability details

Description

Collateral.sol exposes a permissioned withdraw function:

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

An approved manager can withdraw the desired amount provided the hook executes successfully.

The hooks implementation is:

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

The code above checks the remaining collateral amount is above a calculated minimum reserve. getMinReserve():

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

It returns the current total deposited amount multiplied by minReservePercentage. The issue is that owner can immediately change minReservePercentage to zero:

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

The combination of setting minimum percentage to zero, and allowing withdraw if leaving at least minimum percentage, allows owner to instantly steal the entire reserve held in Collateral.sol.

Impact

Compromised manager + Withdraw hook manager can steal entire Collateral.sol reserves.

Tools Used

Manual audit

Recommended Mitigation Steps

Don't allow minReservePercentage to drop below a constant high number.

[hansfriese]

duplicate of #254

[Picodes]

So this holds only if the compromised address has bothSET_MIN_RESERVE_PERCENTAGE_ROLE and MANAGER_WITHDRAW_ROLE, so assuming the roles are split across contracts, this is less powerful than #254

To me this issue does not describe a privilege escalation scenario as both roles work as intended, so downgrading to QA

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-c

[trust1995]

Just to make sure, if this submission is graded C and other downgraded issues link to this submission, will I get no rewards for them as well?