Revert Lend - Giorgio's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L368-L375

Vulnerability details

Impact

Attacker will take advantage of inacurate prices returned for negative ticks.

Proof of Concept

The _getReferencePoolPriceX96 below does not take into account negative ticks.

            uint32[] memory secondsAgos = new uint32[](2);
            secondsAgos[0] = 0; // from (before)
            secondsAgos[1] = twapSeconds; // from (before)
            (int56[] memory tickCumulatives,) = pool.observe(secondsAgos); // pool observe may fail when there is not enough history available (only use pool with enough history!)
     @>     int24 tick = int24((tickCumulatives[0] - tickCumulatives[1]) / int56(uint56(twapSeconds))); //@note here can be negative 
            sqrtPriceX96 = TickMath.getSqrtRatioAtTick(tick);

This oversight poses a risk, especially when (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0, potentially resulting in a tick value that is larger than it should be. Such discrepancies can create opportunities for price manipulations and arbitrage.

To address this issue, it's crucial to ensure that negative tick values are properly handled and rounded down to avoid inaccuracies in determining the tick value. This correction will enhance the accuracy and reliability of the pricing mechanism, reducing the potential for exploitation and manipulation in the system.

Tools Used

Manual review

Recommended Mitigation Steps

Copy the uniswap implementation and add this line.

if (tickCumulatives[1] - tickCumulatives[0] < 0 && (tickCumulatives[1] - tickCumulatives[0]) % secondsAgo != 0) tick --;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L741-L743 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L1078-L1080

Vulnerability details

When performing liquidation the liquidator will fill the LiquidateParams containing the liquidation details. The issue here is that instead of sending the liquidation rewards to the LiquidateParams.recipient, the rewards will be sent to msg.sender.

Impact

The liquidation rewards will be sent to msg.sender instead of the recipient, any external logic that relies on the fact that the liquidation rewards will be sent to recipient won't hold, this will influence the protocol's composability.

Proof of Concept

In order to keep the system safe the liquidator can and is incentivised to liquidate unhealthy positions. To do so the liquidate() function will be fired with the appropriate parameters. One of those parameters is the address recipient;, the name is quite intuitive for this one, this is where the liquidation rewards are expected to sent.

But If we follow the liquidation() function logic, the rewards will not be sent to recipient address. This piece of code handles the reward distribution.

(amount0, amount1) =
            _sendPositionValue(params.tokenId, state.liquidationValue, 
@>  state.fullValue, state.feeValue, msg.sender); 

We can see that msg.sender is being used instead of params.recipient.

Tools Used

Manual review

Recommended Mitigation Steps

The mitigation is straight forward.

Use params.recipient instead of msg.sender for that specific call.

     (amount0, amount1) =
            _sendPositionValue(params.tokenId, state.liquidationValue, 
 --    state.fullValue, state.feeValue, msg.sender); 
 ++    state.fullValue, state.feeValue, params.recipient); 

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L702-L705

Vulnerability details

Liquidation are a way to keep the protocol in a healthy state and clear up bad debt. If users positions are unhealthy other users are entitled to liquidate them, but can they ? The debtor can avoid being liquidated by repaying an insignificant amount while still maintaining an unhealthy position.

Impact

Impossible liquidation, mounting bad debt, and looming insolvency threatens the protocol.

Proof of Concept

When initiating a liquidation, the protocol will asses the debt shares that the position holds.

 uint256 debtShares = loans[params.tokenId].debtShares;

However if these exactly don't match with the debt that the liquidator provided, the process will revert.

        if (debtShares != params.debtShares) {
            revert DebtChanged();
        }

This is problematic because an malicious user could front run the liquidation transaction by adding 1 wei collateral to the position to deny liquidation.

This exploit can be achieved through the repay()

    function repay(uint256 tokenId, uint256 amount, bool isShare) external override {
        _repay(tokenId, amount, isShare, "");
    }

The calls the _repay() function

       uint256 loanDebtShares = loan.debtShares - shares;
  @>    loan.debtShares = loanDebtShares;
        debtSharesTotal -= shares;

enabling the exploiter to change the loan.debtShares value, and consequentially making the liquidation process revert because debtShares != params.debtShares.

Tools Used

Manual review

Recommended Mitigation Steps

In order to mitigate this issue, it is best advised to remove this check :

        if (debtShares != params.debtShares) {
            revert DebtChanged();
        }

As long as the position is unhealthy it should be liquidatable.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L363-L368

Vulnerability details

When implemented correctly, the twap oracle makes it very expensive to manipulate prices, because it will aggregate prices over time. In the current implementation twap periods are lose and may be set 0. This will expose the oracle to easy flash loan manipulations and should never happen.

Impact

Price can be manipulated, exploiting the pool and other liquidity providers

Proof of Concept

Here,

        if (twapSeconds == 0) {
            (sqrtPriceX96,,,,,,) = pool.slot0();
        } else {

In this bit of code the protocol uses slot0 to access the price data which is very easy to manipulate. An attacker could manipulate the price with a flashloan sell assets at an inflated price and destroy the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Do not use slot0 for price fetching, and use a meaningful twapSeconds, such as 1800, 30 minutes, which is a standard. I saw in the tests that 60 secs were being used which is better than slot0 but still easely maniable(cheap compared to 30 minutes).

Assessed type

Oracle