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

0 stars 1 forks source link

Possible bankrun scenario in collateral token #103

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L65 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L80

Vulnerability details

In the Collateral.sol contract, there is a possibility to withdraw the base token using the managerWithdraw function. This withdrawal are limited by the ManagerWithdrawHook.sol contract. However, in ManagerWithdrawHook.sol, minReservePercentage can be set to anything from 0 to 100, which means that we are accepting the possibility of having a collateral token that is not fully backed.

But in a scenario where the collateral token is not fully backed, it is rational for users to withdraw their funds immediately, since there would not be enough base asset to go around. This creates a race, where the fastest users will be made whole, while the slowest will get nothing and lose their funds entirely.

Recommended Mitigation Steps

There are multiple approaches to addressing this issue:

Set minReservePercentage to always be equal to 100. Optionally, create a function that would withdraw all funds in case of an emergency, so that the collateral is either 0% or >= 100% backed. Create a withdraw function similar to the one in the ERC-4626 vault, where users get their share of the underlying asset. This will allow for an undercollaterized vault, while still distributing the base asset fairly among users.

hansfriese commented 1 year ago

duplicate of #254

Picodes commented 1 year ago

There is currently this possibility: in case the contract is under collateralized, the manager can withdraw the funds and do a fair settlement. It would indeed be better than a bank run scenario. This falls within QA to me as it's more an interesting suggestion than a vulnerability.

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

c4-judge commented 1 year ago

Picodes marked the issue as grade-a

ramenforbreakfast commented 1 year ago

While this scenario is correct, it is how the Collateral architecture is designed, it is a fractional reserve system that is subject to all the limitations of a fractional reserve sort of architecture, where there is significant trust on the entity responsible for liabilities (customer deposits).

Going to dispute, since returning users a share of the underlying asset would be a completely different reserve/liability model.

c4-sponsor commented 1 year ago

ramenforbreakfast marked the issue as sponsor disputed

c4-judge commented 1 year ago

Picodes marked the issue as grade-b

Picodes commented 1 year ago

I'll accept the report as a grade-b QA report, as the warden is not really suggesting to totally change the system, but more highlighting the fact that it could provide an additional layer of safety for users to have some automatic pausing in case there is a loss of funds