0x52 - Relying solely on oracle base slippage parameters can cause significant loss due to sandwich attacks

[sherlock-admin]

0x52

medium

Relying solely on oracle base slippage parameters can cause significant loss due to sandwich attacks

Summary

AaveLeverageStrategyExtension relies solely on oracle price data when determining the slippage parameter during a rebalance. This is problematic as chainlink oracles, especially mainnet, have upwards of 2% threshold before triggering a price update. If swapping between volatile assets, the errors will compound causing even bigger variation. These variations can be exploited via sandwich attacks.

Vulnerability Detail

AaveLeverageStrategyExtension.sol#L1147-L1152

function _calculateMinRepayUnits(uint256 _collateralRebalanceUnits, uint256 _slippageTolerance, ActionInfo memory _actionInfo) internal pure returns (uint256) {
    return _collateralRebalanceUnits
        .preciseMul(_actionInfo.collateralPrice)
        .preciseDiv(_actionInfo.borrowPrice)
        .preciseMul(PreciseUnitMath.preciseUnit().sub(_slippageTolerance));
}

When determining the minimum return from the swap, _calculateMinRepayUnits directly uses oracle data to determine the final output. The differences between the true value and the oracle value can be systematically exploited via sandwich attacks. Given the leverage nature of the module, these losses can cause significant loss to the pool.

Impact

Purely oracle derived slippage parameters will lead to significant and unnecessary losses

Code Snippet

AaveLeverageStrategyExtension.sol#L1147-L1152

Tool used

Manual Review

Recommendation

The solution to this is straight forward. Allow keepers to specify their own slippage value. Instead of using an oracle slippage parameter, validate that the specified slippage value is within a margin of the oracle. This gives the best of both world. It allows for tighter and more reactive slippage controls while still preventing outright abuse in the event that the trusted keeper is compromised.

[ckoopmann]

While raising a valid point regarding Chainlink price change trigger, this still seems the best way to set slippage.

Letting keepers set their own min/max amount is definitely not safe as they are not necessarily trusted. (Especially in the case of the "ripcord" function which can be called by anyone)

[pblivin0x]

The proposed solution to allow keepers to specify their own slippage value and validate that the specified slippage value is within a margin of the oracle looks good to me

[ckoopmann]

Ah yes, I missed the validate that the specified slippage value is within a margin of the oracle part of the suggestion, which does make sense and would probably slightly decrease slippage risk. However I'm not sure if it is worth the resulting changes in our keeper infrastructure and having a different keeper interface from other leverage modules.

Overall changing this to "confirmed" but unsure yet of wether we will actually fix it.

[0xffff11]

Great catch and fix seems reasonable. Valid medium

[ckoopmann]

After extensive deliberation we decided to not fix this, as the suggested changes don't seem to justify the effort and might in fact open new attack vectors / issues.

Given that we have had multiple years of experience with this setup in other leveraged tokens, without encountering issues the more conservative approach seems to keep it as is.