code-423n4 / 2024-02-wise-lending-findings

8 stars 6 forks source link

Exiting a farm on mainnet assumes a peg of 1:1 when swapping stETH for ETH #304

Open c4-bot-9 opened 4 months ago

c4-bot-9 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L194-L199 https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarmLeverageLogic.sol#L293-L308

Vulnerability details

Impact

When stETH depegs from ETH, the swaps on Curve will revert due to requesting a higher amountOut than what the curves pool will give.

Proof of Concept

When exiting a farm on mainnet, the requested tokensOut is set as stETH for redeeming the SY tokens on the PENDLE_SY contract. Once the PowerFarm has on its balance the stETH tokens, it does a swap from stETH to ETH using the Curves protocol.

The problem is that the implementation is assuming a peg of 1 ETH ~= 1 stETH. Even though both tokens have a tendency to keep the peg, this hasn't been always the case as it can be seen in this dashboard. There have been many episodes of market volatility that affected the price of stETH, notably the one in last June when stETH traded at ~0.93 ETH.

When computing the _minOutAmount, the PowerFarm calculates the ethValue of the received stETH tokens by requesting the price of the WETH asset, and then it applies the reverAllowedSpread to finally determine the _minOutAmount of ETH tokens that will be accepted from the swap on Curves

function _logicClosePosition(
    ...
)
    private
{
    ...

    //@audit-info => When exiting a farm on mainnet, the `tokenOut` is set to be `stETH`
    address tokenOut = block.chainid == 1
        ? ST_ETH_ADDRESS
        : ENTRY_ASSET;

    ...

    uint256 ethAmount = _getEthBack(
        tokenOutAmount,
        _getTokensInETH(
            //@audit-info => When exiting a farm on mainnet, the price of the `tokenOut` is requested as if it were the price of `WETH`
            //@audit-issue => Here is where the code assumes a peg of `1 stETH to be 1 ETH`
            block.chainid == 1
                ? WETH_ADDRESS
                : ENTRY_ASSET,
            tokenOutAmount
        )
            * reverseAllowedSpread
            / PRECISION_FACTOR_E18
    );

    ...
}

function _getEthBack(
    uint256 _swapAmount,
    uint256 _minOutAmount
)
    internal
    returns (uint256)
{
    if (block.chainid == ETH_CHAIN_ID) {
        //@audit-info => Does a swap of stETH for ETH on the Curves exchange
        return _swapStETHintoETH(
            _swapAmount,
            _minOutAmount
        );
    }

    ...
}

function _swapStETHintoETH(
    uint256 _swapAmount,
    uint256 _minOutAmount
)
    internal
    returns (uint256)
{
    return CURVE.exchange(
        {
            fromIndex: 1,
            toIndex: 0,
            exactAmountFrom: _swapAmount,
            //@audit-info => minimum amount of ETH that the PowerFarm will accept for swapping `exactAmountFrom` of `stETH` tokens!
            minReceiveAmount: _minOutAmount
        }
    );
}

Tools Used

Manual Audit & H-06 finding on Asymmetry Finance contest & Asymmetry's mitigation review & Asymmetry's mitigation review

Recommended Mitigation Steps

The recommendation would be to implement a mitigation similar to the one implemented on the referenced issues. Basically, fetch the current price of stETH from a Chainlink Oracle and multiply the minOutAmount by the current price of stETH. In this way, the minOutAmount that is sent to the Curves exchange will now be within the correct limits based on the current price of stETH.

Assessed type

Context

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as sufficient quality report

GalloDaSballo commented 4 months ago

Worth reviewing

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as primary issue

vonMangoldt commented 3 months ago

Since we have ethValueBefore and after we could also set it to 0 and no harm done thus invalid

c4-judge commented 3 months ago

trust1995 marked the issue as satisfactory

trust1995 commented 3 months ago

@vonMangoldt Would need additional description for the issue described is not actually effective, as from my analysis I can't find a counterexample.

c4-judge commented 3 months ago

trust1995 marked the issue as selected for report

thebrittfactor commented 2 months ago

For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.

vm06007 commented 2 months ago

additionally: Team decided to use PendleRouter (https://github.com/pendle-finance/pendle-core/blob/master/contracts/core/PendleRouter.sol) instead of Curve moving forward during farm exit.