Manager can get around min reserves check, draining all funds from Collateral.sol

[code423n4]

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().

[Picodes]

How would you implement your mitigation ? I don't see how it'd work considering that this would also your PoC would also hold using an other address to do the trick

[Picodes]

From what I understand, although it's not clear from the documentation or the code, this minReserve requirement is here to keep some funds in the contract to allow for withdrawals but does not provide any additional safety, and it should be clear for users that a compromised manager would immediately lead to a loss of all funds

[Picodes]

I'll merge all issues regarding the manager being able to withdraw all funds, regardless of the method, the core issue being that the managerWithdrawHook check is easily bypassable.

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

ramenforbreakfast marked the issue as sponsor disputed

[c4-sponsor]

ramenforbreakfast marked the issue as sponsor confirmed

[trust1995]

If this is where centralization issues are concentrated, I would suggest dropping the severity to Med.

[Picodes]

Indeed, Med severity is more appropriate for this centralization issue. So far it was only the deduping phase.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[c4-judge]

Picodes marked the issue as not selected for report

[c4-judge]

Picodes marked the issue as selected for report

[Picodes]

Flagging as best for this centralization issue, combined with the other finding by the same warden https://github.com/code-423n4/2022-12-prepo-findings/issues/255