It is possible that strategy cannot be harvested and its loss fails to be reported or realized

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L109-L138
https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L114-L142
https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L493-L560
https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperVaultV2.sol#L432-L453

Vulnerability details

Impact

When calling the ReaperBaseStrategyv4.harvest function, the debt can be positive because the availableCapital returned by calling IVault(vault).availableCapital() can be negative. Then, the debt is used to call the ReaperStrategyGranarySupplyOnly._harvestCore function.

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/abstract/ReaperBaseStrategyv4.sol#L109-L138

    function harvest() external override returns (int256 roi) {
        _atLeastRole(KEEPER);
        int256 availableCapital = IVault(vault).availableCapital();
        uint256 debt = 0;
        if (availableCapital < 0) {
            debt = uint256(-availableCapital);
        }

        uint256 repayment = 0;
        if (emergencyExit) {
            ...
        } else {
            (roi, repayment) = _harvestCore(debt);
        }

        debt = IVault(vault).report(roi, repayment);
        ...
    }

When calling the ReaperStrategyGranarySupplyOnly._harvestCore function, if totalAssets < allocated is true, then the roi is first set to -int256(allocated - totalAssets) and is negative. The amountFreed and loss returned by calling _liquidatePosition(toFree), where toFree is the _debt, would add up to the _debt. Since loss can be positive, roi is further reduced by the loss after executing roi -= int256(loss). However, this can cause the -_roi to be larger than the vault's token amount that is allocated to the strategy.

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Vault/contracts/ReaperStrategyGranarySupplyOnly.sol#L114-L142

    function _harvestCore(uint256 _debt) internal override returns (int256 roi, uint256 repayment) {
        ...

        uint256 allocated = IVault(vault).strategies(address(this)).allocated;
        uint256 totalAssets = balanceOf();
        uint256 toFree = _debt;

        if (totalAssets > allocated) {
            ...
        } else if (totalAssets < allocated) {
            roi = -int256(allocated - totalAssets);
        }

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

Calling the ReaperVaultV2.report function then calls the ReaperVaultV2._reportLoss function because the _roi is negative. Since the roi was further reduced by the loss in the ReaperStrategyGranarySupplyOnly._harvestCore function that caused the the -_roi to be more than the vault's token amount that is allocated to the strategy, calling the ReaperVaultV2._reportLoss function would revert when executing require(loss <= allocation, "Strategy loss cannot be greater than allocation"). Hence, in this situation, calling the ReaperBaseStrategyv4.harvest function for the corresponding strategy reverts in which such strategy cannot be harvested and its loss fails to be reported or realized.

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

    function report(int256 _roi, uint256 _repayment) external returns (uint256) {
        LocalVariables_report memory vars;
        vars.stratAddr = msg.sender;
        StrategyParams storage strategy = strategies[vars.stratAddr];
        require(strategy.activation != 0, "Unauthorized strategy");

        if (_roi < 0) {
            vars.loss = uint256(-_roi);
            _reportLoss(vars.stratAddr, vars.loss);
        } else if (_roi > 0) {
            ...
        }

        ...
    }

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

    function _reportLoss(address strategy, uint256 loss) internal {
        StrategyParams storage stratParams = strategies[strategy];
        // Loss can only be up the amount of capital allocated to the strategy
        uint256 allocation = stratParams.allocated;
        require(loss <= allocation, "Strategy loss cannot be greater than allocation");

        ...
    }

Proof of Concept

The following steps can occur for the described scenario.

1. When calling the ReaperBaseStrategyv4.harvest function, calling IVault(vault).availableCapital() can return -3e6 so the debt is set to 3e6.
2. When calling the ReaperStrategyGranarySupplyOnly._harvestCore function, the totalAssets can be 1e6, the allocated can be 5e6, and totalAssets < allocated is true if the vault currently has 5e6 tokens allocated to the strategy and the current token amount controlled by the strategy is 1e6.
3. In the ReaperStrategyGranarySupplyOnly._harvestCore function, the roi is first set to -4e6 after executing roi = -int256(allocated - totalAssets).
4. Also in the ReaperStrategyGranarySupplyOnly._harvestCore function, the amountFreed can be 1e6 and the loss can be 2e6 after calling _liquidatePosition(toFree), where toFree is the _debt that is 3e6. Then, the roi is further reduced to -6e6 after executing roi -= int256(loss).
5. When calling the ReaperVaultV2.report function, the vars.loss is set to uint256(-_roi), which is 6e6.
6. Then, calling the ReaperVaultV2._reportLoss function with the loss input being 6e6 reverts because the loss is bigger than the strategies[strategy].allocated that is still 5e6.
7. As a result, the corresponding strategy cannot be harvested and its loss fails to be reported or realized.

Tools Used

VSCode

Recommended Mitigation Steps

The ReaperStrategyGranarySupplyOnly._harvestCore function can be updated to not execute roi -= int256(loss) for the described scenario.

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as primary issue

[tess3rac7]

Duplicates #488 . Leaving up to judge how to handle.

[c4-sponsor]

tess3rac7 requested judge review

[c4-judge]

trust1995 marked the issue as duplicate of #488

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[rbserver]

Hi @trust1995,

Unlike #488 and #124, this report identifies roi -= int256(loss) in the ReaperStrategyGranarySupplyOnly._harvestCore function as the root cause, which is similar to #752.

Moreover, this report shows that calling the ReaperVaultV2.report function for the affected strategy would revert to demonstrate the impact in which such affected strategy's loss cannot be realized. For such affected strategy, calling the ReaperVaultV2.report function should not revert because its loss needs to be realized to ensure that the vault's accounting states are updated correctly. For instance, for the example of this report's POC, if roi -= int256(loss) in the ReaperStrategyGranarySupplyOnly._harvestCore function is not executed, then calling the ReaperVaultV2.report function would not revert because its _roi input value would be -4e6 instead of -6e6. Thus, this report's concern about the failure of realizing the affected strategy's loss is similar to your comment about debt = IVault(vault).report(roi, repayment) in #752.

Hence, I would like to ask if this report can be considered as a duplicate of #752.

Thanks!

[c4-judge]

This previously downgraded issue has been upgraded by trust1995

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as duplicate of #752