All deposited user assets may be drained

[code423n4]

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.

[Picodes]

This is a centralization issue, and a design choice, although it's right that it should be properly documented and disclosed

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[c4-sponsor]

davidprepo marked the issue as sponsor disputed

[ramenforbreakfast]

Disputing this issue because it is just pointing out 1 centralization issue that has already been mentioned by countless other more thorough submissions.