A manager can withdraw all amount and DoS withdrawals

[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
https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/WithdrawHook.sol#L58

Vulnerability details

Impact

Manager for Collateral smart contract has complete role over collateral funds and is able to withdraw all of it

The assumption is that the manager is honest, even though there is a big risk with wrongly configured MultiSig (e.g. admin that can circumvent M of N votes to take an action), keys compromise, even moving funds to unepected address which would lead to funds being lost.

Proof of Concept

Manager can call managerWithdraw to get all the funds:

80:  function managerWithdraw(uint256 _amount) external override onlyRole(MANAGER_WITHDRAW_ROLE) nonReentrant {
81:    if (address(managerWithdrawHook) != address(0)) managerWithdrawHook.hook(msg.sender, _amount, _amount);
82:    baseToken.transfer(manager, _amount);
83:  }

or halt withdrawals:

Colateral.sol
73:  withdrawHook.hook(msg.sender, _baseTokenAmount, _baseTokenAmountAfterFee);

WithdrawalHook.sol
//withdrawalsAllowed is set by manager account 
58:    require(withdrawalsAllowed, "withdrawals not allowed");//@audit-issue LOW - by default it's false, any withdrawal will fail

Tools Used

VS Code

Recommended Mitigation Steps

If only function of this is to move funds to some yield bearing protocol (e.g. Aave, DEXes), please implement supposed functionality in the smart contract. Otherwise, it's best to remove the function completely.

[hansfriese]

duplicate of #254

[c4-judge]

Picodes marked the issue as duplicate of #254

[Picodes]

Also dup of #305

[c4-judge]

Picodes marked the issue as satisfactory