managerWithdraw can be called when manager isn't set, wiping all user funds

[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

The manager role in Collateral.sol must be set manually. It isn't included in the constructor or initialize functions.

It also isn't necessary in order to set the MANAGER_WITHDRAW_ROLE.

In the case where the MANAGER_WITHDRAW_ROLE is set and manager is not, the user is able to call managerWithdraw().

This will send all requested funds to the zero address, where they will be irretrievable.

Proof of Concept

manager isn't set anywhere in the contract except the setManager function.

There is no requirement that this function must have been called in order to call managerWithdraw().

The result is that any call to managerWithdraw() before manager is set will destroy all funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in this function that the manager is set before sending funds:

require(manager != address(0), 'manager must be set to send funds');

[Picodes]

This is theoretically possible but is not a high severity finding. It'd require centralization + errors. So downgrading to low as it's more a safety check.

[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

[ghost]

System will never be left in this state