Upgraded Q -> M from #334 [1671456734920]

[c4-judge]

Judge has assessed an item in Issue #334 as M risk. The relevant finding follows:

Collateral.withdraw allows the manager to withdraw an arbitrary _amount of baseToken from Collateral.

The only check is in the ManagerWithdrawHook.hook call, where it checks the withdrawal does not drop the amount of _baseToken below (depositRecord.getGlobalNetDepositAmount() * minReservePercentage) / PERCENT_DENOMINATOR

A manager and the account holding SET_MIN_RESERVE_PERCENTAGE_ROLE can perform the following steps:

1 - call ManagerWithdrawHook.setMinReservePercentage(PERCENT_DENOMINATOR) 2 - call Collateral.managerWithdraw(baseToken.balanceOf(address(this)))

And rug all the baseToken in Collateral, stealing from depositors who are then unable to withdraw the baseToken they are entitled to.

The reason this attack is currently possible is because Collateral.deposit does not directly send the tokens to StrategyController. After a user deposit, the tokens are sitting in Collateral until the tokens are transferred to StrategyController, making this attack possible.

Recommendation Looking at the deposit flow diagram, a possible mitigation is to make the deposit process atomic, ensuring tokens are deposited into the Strategy in the same transaction as the users deposit into Collateral, to make the attack impossible (or at least lower its impact).

[c4-judge]

Picodes marked the issue as duplicate of #254

[c4-judge]

Picodes marked the issue as satisfactory