Ethos Reserve contest - 0xRobocop's results

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/LQTY/LQTYStaking.sol#L181

Vulnerability details

Impact

The function increaseF_Collateral at the LQTYStaking.sol contract is meant to be called by other contracts on the system to distribute redemption fees for a given collateral to the Oath/WETH lp stakers. This is achieved by incrementing the F_Collateral[collateral] value, the code looks like this:

function increaseF_Collateral(address _collateral, uint _collFee) external override {
        _requireCallerIsTroveManagerOrActivePool();
        uint collFeePerLQTYStaked;
     
        if (totalLQTYStaked > 0) {collFeePerLQTYStaked = _collFee.mul(DECIMAL_PRECISION).div(totalLQTYStaked);}

        F_Collateral[_collateral] = F_Collateral[_collateral].add(collFeePerLQTYStaked);
        emit F_CollateralUpdated(_collateral, F_Collateral[_collateral]);
    }

If the token used for _collFee have a low number of decimals and a high USD value, then the result for collFeePerLQTYStaked can be zero. This will cause the F_Collateral[_collateral] to not get incremented as expected.

Proof of Concept

The original LQTYStaking.sol contract from Liquity was meant to only work for ETH as collateral. ETH or WETH have 18 decimals so they are safe.

In the case of Ethos Reserve they have made clear that WBTC will be one of the allowed collaterals. WBTC have 8 decimals and a high USD value. Take into account the following scenario:

WBTC usd value == 23,900 WBTC decimals = 8 DECIMAL_PRECISION == 1e18

LQTY token is expected to be Balancer Lp tokens for the 80/20 Oath/Weth pool. The 80/20 Bal/Weth pool have 14_000_000e18 lp tokens as total supply, so for this example I will take the same value for totalLQTYStaked.

Because WBTC have a high usd value, it is rational to assume that the amount of redemption fees for this collateral will be less than a complete WBTC. Let's use 0.02 WBTC (it is still a considerable amount). Then we have _collFee == 2_000_000.

Substituting all the values on the formula, we have:

collFeePerLQTYStaked = (2_000_000 * 1e18) / 14_000_000e18

Because 2M is less than 14M the result is zero, even though the contract received 0.02 WBTC as redemption fee.

Tools Used

Manual Review

Recommended Mitigation Steps

Add another precision factor for collateral types.

Lines of code

https://github.com/code-423n4/2023-02-ethos/blob/main/Ethos-Core/contracts/ActivePool.sol#L251

Vulnerability details

Background

When a strategy losses collateral, it reports the loss back to its parent vault, this is done via the _lossReport() function:

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");

        if (totalAllocBPS != 0) {
            // reduce strat's allocBPS proportional to loss
            uint256 bpsChange = Math.min((loss * totalAllocBPS) / totalAllocated, stratParams.allocBPS);

            // If the loss is too small, bpsChange will be 0
            if (bpsChange != 0) {
                stratParams.allocBPS -= bpsChange;
                totalAllocBPS -= bpsChange;
            }
        }

        // Finally, adjust our strategy's parameters by the loss
        stratParams.losses += loss;
        stratParams.allocated -= loss;
        totalAllocated -= loss;
    }

We can see that the vault updates its storage to account for the loss, for example it reduces the totalAllocated variable. This directly affects what the balanceOf() function returns:

function balance() public view returns (uint256) {
        return token.balanceOf(address(this)) + totalAllocated;
}

The problem is that this loss is not bubble up to the ActivePool.sol contract. Which has a mapping (address => uint256) public yieldingAmount; that tracks the amount of collateral that the ActivePool contract has allocated to a given vault for yield farming purposes. And another mapping (address => uint256) internal collAmount; which tracks the amount of collateral the ActivePool is managing.

Impact

Each time the ActivePool contract pulls collateral in or sends collateral out, it triggers the _rebalance function. The next code snippet is taken from the _rebalance function:

// how much has been allocated as per our internal records?
vars.currentAllocated = yieldingAmount[_collateral];

// what is the present value of our shares?
vars.yieldGenerator = IERC4626(yieldGenerator[_collateral]);
vars.ownedShares = vars.yieldGenerator.balanceOf(address(this));
vars.sharesToAssets = vars.yieldGenerator.convertToAssets(vars.ownedShares);

// if we have profit that's more than the threshold, record it for withdrawal and redistribution
vars.profit = vars.sharesToAssets.sub(vars.currentAllocated);

If vars.sharesToAssets were to be smaller than vars.currentAllocated this will revert to prevent an underflow. How these values are calculated?.

(1) vars.currentAllocated is copied from yieldingAmount[_collateral] (2) vars.sharesToAssets is the returned value of the convertToAssets() function on the Vault:

function convertToAssets(uint256 shares) public view override returns (uint256 assets) {
        if (totalSupply() == 0) return shares;
        return (shares * _freeFunds()) / totalSupply();
}

_freeFunds() is calculated as follows:

function _freeFunds() internal view returns (uint256) {
        return balance() - _calculateLockedProfit();
}

And from the background section we know that balanceOf() depends on the current collateral balance of the contract plus the totalAllocated variable.

From this analysis we know that vars.sharesToAssets gets affected each time a strategy losses collateral, and hence the parent vault, while vars.currentAllocated do not since it depends on the yieldingAmount[_collateral] mapping which is not aware of any loss.

So, in the case where the vault has lost collateral, each _rebalance in the ActivePool will revert due to vas.sharesToAssets been smaller than vars.currentAllocated.

Because ActivePool holds the collateral from active troves, a rebalance happens each time someone opens a trove, makes a collateral top-up, a collateral withdrawal, closes a trove or liquidates a trove. All these operations will get DoSed until someone sends to the vault the loss incurred.

Because liquidations cannot happen, the protocol have the risk to end uncollateralized, which is the most undesired state on over-collateralized lending protocols.

Proof of Concept

At time t0 the ActivePool have allocated 20 WETH of collateral to yield farm, and have received 20 shares for that. At this point yieldingAmount[WETH] returns 20 WETH.

At time t1 a strategy of the WETH vault reports a loss of 1 WETH. Then the totalAllocated variable of the Vault will get reduced by 1 WETH. At this point the 20 shares converted to assets is only 19 WETH.

At time t2 there is at least one Trove that can be liquidate, because ICR < MCR or the protocol is on recovery mode and ICR < CCR. Alice tries to liquidate this trove, the liquidation will trigger a rebalance on the ActivePool.

The rebalance of the pool will fail because shareToAssets == 19 WETH and yieldingAmount[WETH] == 20 WETH .

Tools Used

Manual Review

Recommended Mitigation Steps

Short term:

From the background section we know the Active pool have the mappings collAmount[_collateral] and yieldingAmount[_collateral]. This first one tracks all the collateral the ActivePool is managing and the latter is the collateral that has been allocated for yield farming.

Each time a strategy reports a loss to its parent vault, this loss must be bubble up to these mappings in the ActivePool contract.

Long term:

The ethos-reserve product is achieved by merging the liquity protocol and a vault-style protocol like yearn.

The liquity protocol was designed with the idea that the collateral backing the LUSD loans cannot get lost. The only way to get collateral out of the contract is if the ICR of each trove and the TCR allows it or during liquidations. In other words, the only way to get collateral out is by following the rules of the protocol that ensures the LUSDDebt remains properly collateralized.

A protocol like yearn, looks to invest capital to earn some yield, but it has the risk to lose a percentage of it.

In other words, all the allowed operations at a certain time on the liquity protocol are bounded by the price of the collateral, and they dont get worried by some collateral balance reduction unless there is a reduction on the LUSD debt or the price of the collateral going up.

When merging these two protocols, the design must take into account that is possible to lose collateral balance without a reduction of LUSD debt or an increment on the price of the collateral.