code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L80-L83 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L17 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L41

Vulnerability details

Impact

hook function in ManagerWithdrawHook is used for ensuring that the amount remaining after withdrawn by manager is above certain threshold(returned by getMinReserve). However, manager can take advantage of this when seeing large withdrawal by user and frontrunning transaction by withdrawing more tokens. If manager would have withdrawn after the withdraw by user, it would receive less tokens.

Proof of Concept

assume minReservePercentage = 50%. There are 100 tokens in the reserve. And some user sends the transaction to withdraw 20 tokens. After withdrawal by user, manager will only be able to withdraw (100 - 20) 0.5 = 40 tokens but if manager withdraws first, then it will be able to withdraw (100 0.5) = 50 tokens and after withdrawal by user, the reserve will fall below minimum reserve.

Tools Used

Manual review

Recommended Mitigation Steps

Use timelock for withdrawal by manager so that it can't frontrun other users.

Picodes commented 1 year ago

Low severity as the impact isn't significant compared to just withdrawing.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

Picodes marked the issue as grade-b

ramenforbreakfast commented 1 year ago

This is fine, reserve is intended to prevent manager from withdrawing beyond the reserve, it is not necessarily a undesirable situation that a user withdraws when Collateral is below reserve requirement.

c4-sponsor commented 1 year ago

ramenforbreakfast marked the issue as sponsor disputed

code-423n4 / 2022-12-prepo-findings