Hacked admin or malicious admin can immediately steal all baseToken in Collateral

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L80-L83
https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L85-L88
https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L112-L115

Vulnerability details

Impact

Hacked admin or malicious admin can immediately steal all baseToken that users deposit in Collateral.

If Collateral is deployed as a upgradable proxy, the collateral contract admin could also steal all assets approved to this contract address.

Proof of Concept

Hacked admin or malicious admin can steal the assets by following these steps:

1. Use the DEFAULT_ADMIN_ROLE: grant his own account X roles(MANAGER_WITHDRAW_ROLE, SET_MANAGER_ROLE, SET_MANAGER_WITHDRAW_HOOK_ROLE), and accept them.
2. Use account X: call Collateral.setManagerWithdrawHook() with _newManagerWithdrawHook = 0 to disable ManagerWithdrawHook.
3. Use account X: call Collateral.setManager() with _newManager = X.
4. Use account X: call Collateral.managerWithdraw() to transfer all baseToken to X. (becase managerWithdrawHook is 0 and manager is X now)

If Collateral is deployed as a proxy(like uups, 1967), the proxy admin could steal all baseToken in this contract and all assets approved to this contract by upgrading Collateral to a new malicious contract.

Tools Used

VS Code

Recommended Mitigation Steps

For the Collateral contract, the following change is recommended:

• managerWithdrawHook should be set to a valid contract at initialize()
• Add a timelock to disable Collateral.managerWithdraw() for some time when either managerWithdrawHook or manager changes.

For proxy contract, two options are recommended:

1. Deploy Collateral behind a non-upgradable proxy.
2. The upgradable proxy should have a timelock when upgrade, give users enough time to withdraw and cancel their approvals before some malicious action becomes possible.

[hansfriese]

duplicate of #254

[c4-judge]

Picodes marked the issue as duplicate of #254

[c4-judge]

Picodes marked the issue as satisfactory