prePO contest - obront's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/WithdrawHook.sol#L53-L79

Vulnerability details

Impact

When a manager withdraws funds from Collateral.sol, there is a check in the managerWithdrawHook to confirm that they aren't pushing the contract below the minimum reserve balance.

require(collateral.getReserve() - _amountAfterFee >= getMinReserve(), "reserve would fall below minimum");

However, a similar check doesn't happen in the withdraw() function.

The manager can use this flaw to get around the reserve balance by making a large deposit, taking a manager withdrawal, and then withdrawing their deposit.

Proof of Concept

Imagine a situation where the token has a balance of 100, deposits of 1000, and a reserve percentage of 10%. In this situation, the manager should not be able to make any withdrawal.

But, with the following series of events, they can:

• Manager calls deposit() with 100 additional tokens
• Manager calls managerWithdraw() to pull 100 tokens from the contract
• Manager calls withdraw() to remove the 100 tokens they added

The result is that they are able to drain the balance of the contract all the way to zero, avoiding the intended restrictions.

Tools Used

Manual Review

Recommended Mitigation Steps

Include a check on the reserves in the withdraw() function as well as managerWithdraw().

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