Loss in a strategy could prevent withdraws.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L359
https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L392
https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L436

Vulnerability details

Impact

When a withdraw happen on the Vault, if the amount required to be withdraw is greater then the current balance of the Vault, the Vault will try to withdraw allocated funds from the strategies until it reach the required amount to be withdrawn. The problem in such scenario is that everytime the fund are withdraw from a strategy, it will determine if the strategy got a loss or a gain, and in case of a loss we will be reporting this to our own accounting (the vault doesn't blindly trust the strategy). At the moment of reporting this loss, if the loss is greater than the allocation (which should be impossible), the whole transaction will be reverted, so the withdraw.

The strategy is an external contract of the Vault (even if we manage the code, here we use Granary in the current contest which seems safe, but more will be added overtime which could introduce bug) and we cannot fully control what it will return as a loss from a Vault perspective. I agree that it should never return a loss be greater than the allocation, but if that ever happen du to a bug in the strategy (unexpected external condition to the strategy, which is why you added a problematic require in the first place in the vault), it would cause a problem for the entire vault withdraw process, which then could be problematic as depositor or active pool cannot get funds back in a timely manner, until the strategy would be having a positive ROI. In case the strategy cannot come with a positive ROI, fund would be locked into the strategy forever, which is a direct impact on funds, which is why I clasify this issue as High.

Proof of Concept

For example, if you simulate the existing Granary strategy to behave badly (by simulating the behavior I explained), and run the following test, the issue will be happening and the test will fail.

diff --git a/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol b/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
index 67fc601..03c82b2 100644
--- a/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
+++ b/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol
@@ -138,7 +138,8 @@ contract ReaperStrategyGranarySupplyOnly is ReaperBaseStrategyv4, VeloSolidMixin

         (uint256 amountFreed, uint256 loss) = _liquidatePosition(toFree);
         repayment = MathUpgradeable.min(_debt, amountFreed);
-        roi -= int256(loss);
+        //roi -= int256(loss);
+        roi = -int256(1);
     }

it.only('should be able to harvest', async function () {

1) Vaults
       Strategy
         should be able to harvest:
     Error: VM Exception while processing transaction: reverted with reason string 'Strategy loss cannot be greater than allocation'
      at ReaperVaultERC4626.report (contracts/ReaperVaultV2.sol:508)
      at ReaperVaultERC4626.report (contracts/ReaperVaultV2.sol:512)
      at ReaperStrategyGranarySupplyOnly.harvest (contracts/abstract/ReaperBaseStrategyv4.sol:133)
      at <UnrecognizedContract>.<unknown> (0x09635f643e140090a9a8dcd712ed6285858cebef)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at HardhatNode._mineBlockWithPendingTxs (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:1819:23)
      at HardhatNode.mineBlock (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:508:16)
      at EthModule._sendTransactionAndReturnHash (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:1522:18)
      at HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:123:18)
      at EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)

Tools Used

Code examination and test from starter-test.js

Recommended Mitigation Steps

• The withdraw operation should not revert because of reporting a loss of a bad strategy. It should move on to the next one until the fund to withdraw are reach, at least in non-emergency mode.
• The current emergency mechanishm would need to be activated and probably liquidated all positions, which should hopefully make the loss disappear among all the other strategy being liquidated. If there is only one strategy, that's problematic. The vault would not be able to retrieve fund from strategy until it get a positive ROI.
• Maybe in emergency mode, _reportLoss could behave differently, and instead of reverting, would re assign the loss at : Math.min(loss, allocation) and move on.

[c4-judge]

trust1995 marked the issue as duplicate of #710

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as duplicate of #488

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by trust1995

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)