Olas - erebus's results

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/liquidity_lockbox.sol#L84

Vulnerability details

Impact

It won't be possible to withdraw any LP token after doing a deposit of $0$ liquidity, leading to withdrawals being effectively freezed.

Proof of Concept

In

liquidity_lockbox, function withdraw

        ...

        uint64 positionLiquidity = mapPositionAccountLiquidity[positionAddress];
        // Check that the token account exists
        if (positionLiquidity == 0) {
            revert("No liquidity on a provided token account");
        }

        ...

the code checks for the existence of a position via the recorded liquidity. This is a clever idea, as querying a non-existant value from a mapping will return $0$. However, in deposit, due to a flawed input validation, it is possible to make positions with $0$ liquidity as the only check being done is for liquidity to not be higher than type(uint64).max:

liquidity_lockbox, function _getPositionData

        ...

        // Check that the liquidity is within uint64 bounds
        if (positionData.liquidity > type(uint64).max) {
            revert("Liquidity overflow");
        }

        ...

As it will pass the input validation inside _getPositionData, the only way for such a tx to revert is in the transfer/mint, which are low-level calls with no checks for success, as stated in my report Missing checks for failed calls to the token program will corrupt user's positions.

Due to the reasons above, this deposit with $0$ liquidity will be treated as a valid one and will be stored inside the mapPositionAccountLiquidity and positionAccounts arrays. If we add the fact that withdrawals are done by looping LINEARLY through positionAccounts:

liquidity_lockbox, function withdraw

    function withdraw(uint64 amount) external {
        address positionAddress = positionAccounts[firstAvailablePositionAccountIndex]; // @audit linear loop
        
        ...

        uint64 positionLiquidity = mapPositionAccountLiquidity[positionAddress];
        // Check that the token account exists
        if (positionLiquidity == 0) { // @audit it will revert here once it reaches the flawed position
            revert("No liquidity on a provided token account");
        }

        ...

        if (remainder == 0) { // @audit if the liquidity after the orca call is 0, close the position and ++ the index
            ...

            // Increase the first available position account index
            firstAvailablePositionAccountIndex++; // @audit it won't reach here as the revert above will roll-back the whole tx
        }
    }

It can be seen that once it encounters such a "fake" deposit with $0$ liquidity provided, it will always revert due to the existence check. As there is no other way to update firstAvailablePositionAccountIndex to bypass the flawed position, withdrawals will be completely freezed.

Recommended Mitigation Steps

Just check for the supplied liquidity to not be $0$ in

liquidity_lockbox, function _getPositionData

        ...
+       // Check that the liquidity > 0
+       if (positionData.liquidity == 0) {
+           revert("Liquidity cannot be 0");
+       }

        // Check that the liquidity is within uint64 bounds
        if (positionData.liquidity > type(uint64).max) {
            revert("Liquidity overflow");
        }

        ...

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/liquidity_lockbox.sol#L377

Vulnerability details

Impact

In liquidity_lockbox, function getLiquidityAmountsAndPositions will return a corrupted value under certain situations, making users withdraw the wrong amount of tokens.

Proof of Concept

The purpose of liquidity_lockbox, function getLiquidityAmountsAndPositions is:

    /// @dev Gets liquidity amounts and position accounts in order to correctly withdraw a specified liquidity amount.

As it is not used in the whole project, then this will be used by the front-end to query the needed arguments to pass to withdraw in order to withdraw the amount specified by the user. The high-level workflow is as follows:

1. Check if the specified amount is higher than the total liquidity
2. Loop through all positions until the cummulative liquidity is equal or higher than the amount
   1. NOTE $\Rightarrow$ If the cummulative liquidity is HIGHER, then store the difference
3. Populate the return values
4. If 2.i was triggered, update the last positionAmounts with such a value

As we can see, step 4 is flawed as the stored amount is HIGHER than the expected one:

    function getLiquidityAmountsAndPositions(uint64 amount)
        external view returns (uint64[] positionAmounts, address[] positionAddresses, address[] positionPdaAtas)
    {
        // @audit step 1
        if (amount > totalLiquidity) {
            revert ("Requested amount is too big for the total available liquidity");
        }

        uint64 liquiditySum = 0;
        uint32 numPositions = 0;
        uint64 amountLeft = 0;

        // @audit step 2
        // Get the number of allocated positions
        for (uint32 i = firstAvailablePositionAccountIndex; i < numPositionAccounts; ++i) {
            address positionAddress = positionAccounts[i];
            uint64 positionLiquidity = mapPositionAccountLiquidity[positionAddress];

            // Increase a total calculated liquidity and a number of positions to return
            liquiditySum += positionLiquidity;
            numPositions++;

            // @audit step 2.1
            // Check if the accumulated liquidity is enough to cover the requested amount
            if (liquiditySum >= amount) {
                amountLeft = liquiditySum - amount;
                break;
            }
        }

        // @audit step 3
        // Allocate the necessary arrays and fill the values
        positionAddresses = new address[](numPositions);
        positionAmounts = new uint64[](numPositions);
        positionPdaAtas = new address[](numPositions);
        for (uint32 i = 0; i < numPositions; ++i) {
            positionAddresses[i] = positionAccounts[firstAvailablePositionAccountIndex + i];
            positionAmounts[i] = mapPositionAccountLiquidity[positionAddresses[i]];
            positionPdaAtas[i] = mapPositionAccountPdaAta[positionAddresses[i]];
        }

        // @audit step 4
        // Adjust the last position, if it was not fully allocated
        if (numPositions > 0 && amountLeft > 0) {
            positionAmounts[numPositions - 1] = amountLeft; // @audit wrong should be -= instead
        }

That is: $amount \overset{?}{=} \sum^{n - 1}_{i=0} p_i \ + amountLeft$

$amount \overset{?}{=} \sum^{n - 1}_{i=0} p_i \ + ( \ \sum^n_{i=0} p_i - amount \ )$

$amount + amount \overset{?}{=} \sum^{n - 1}_{i=0} p_i \ + \sum^n_{i=0} p_i$

$2 * amount \overset{?}{=} 2 * \sum^{n - 1}_{i=0} p_i + p_n$

can only hold if $p_n = 0$ or, in other terms, if there is no amountLeft at all. The visual POC is the next one (with a simplified example):

I have put this as a Med due to Supreme Court resolution (link):

2 — Med:
    - An egregious mistake that doesn’t directly lead to loss of funds (ERCs / Incorrect View Logic, Missing function)

as this is a view function whose return value will be used when calling withdraw, leading to such a function not returning the expected assets in exchange, that is, users will expect $X$ but will receive $Y < X$.

Recommended Mitigation Steps

Update the code responsible of step 4:

liquidity_lockbox, line 377

        // Adjust the last position, if it was not fully allocated
        if (numPositions > 0 && amountLeft > 0) {
-           positionAmounts[numPositions - 1] = amountLeft;
+           positionAmounts[numPositions - 1] -= amountLeft;
        }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/lockbox-solana/solidity/liquidity_lockbox.sol#L277

Vulnerability details

Impact

Vanilla example of missing slippage protection.

Proof of Concept

As any AMM, Orca implements slippage protections by letting the caller specify the minimum amount of input and output tokens to be used in a given operation, so that if the transaction outputs a value less than the expected ones, it will revert preventing users from incurring in unacceptable losses.

In our situation, we are interested in the decreaseLiquidity handler:

decrease_liquidity, lines 18 and 19

pub fn handler(
    ctx: Context<ModifyLiquidity>,
    liquidity_amount: u128,
    token_min_a: u64,
    token_min_b: u64,
) -> Result<()> {
    
    ...

    let (delta_a, delta_b) = calculate_liquidity_token_deltas(
        ctx.accounts.whirlpool.tick_current_index,
        ctx.accounts.whirlpool.sqrt_price,
        &ctx.accounts.position,
        liquidity_delta,
    )?;

    if delta_a < token_min_a {
        return Err(ErrorCode::TokenMinSubceeded.into());
    } else if delta_b < token_min_b {
        return Err(ErrorCode::TokenMinSubceeded.into());
    }

    ...
}

The code snippet above calculates the delta variations of the two tokens amount in the given pool and reverts the whole transaction if the calculated amounts are less than the specified ones. However, if we go to

liquidity_lockbox, function withdraw

        ...

        whirlpool.decreaseLiquidity{accounts: metasDecreaseLiquidity, seeds: [[pdaProgramSeed, pdaBump]]}(amount, 0, 0);

        ...

we see that token_min_a and token_min_b are hard-coded to $0$, meaning that the liquidity decrease operation will accept ANY amount in exchange, even $0$, leading to a loss of funds as MEV exists in Solana too (see here). The mathematical reason is that

$x >= 0, \forall x \in [0, 2⁶⁴-1]$

so the errors

    if delta_a < token_min_a {
        return Err(ErrorCode::TokenMinSubceeded.into());
    } else if delta_b < token_min_b {
        return Err(ErrorCode::TokenMinSubceeded.into());
    }

won't be triggered as

    if delta_a < token_min_a { // @audit delta_a < 0 => false for all delta_a, so no error
        return Err(ErrorCode::TokenMinSubceeded.into());
    } else if delta_b < token_min_b { // @audit delta_b < 0 => false for al delta_b, so no error
        return Err(ErrorCode::TokenMinSubceeded.into());
    }

    // @audit if token_min_a = token_min_b = 0, then delta_a and delta_b can be ANY value, 
    // @audit even 0, which means total loss of funds is possible

meaning there is no slippage protection at all if called with token_min_a = token_min_b = 0 (as it's the case with liquidity_lockbock::withdraw).

Recommended Mitigation Steps

Let users provide the minimum amount of tokens they are willing to use as function arguments and pass them straight to decreaseLiquidity like:

-   function withdraw(uint64 amount) external {
+   function withdraw(uint64 amount, uint64 minA, uint64 minB) external {
        
        ...

-       whirlpool.decreaseLiquidity{accounts: metasDecreaseLiquidity, seeds: [[pdaProgramSeed, pdaBump]]}(amount, 0, 0);
+       whirlpool.decreaseLiquidity{accounts: metasDecreaseLiquidity, seeds: [[pdaProgramSeed, pdaBump]]}(amount, minA, minB);

        ...
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/tokenomics/contracts/GenericBondCalculator.sol#L57

Vulnerability details

Impact

It will cap OLAS payouts much more than intended, as it will be checking two amounts in different basis.

Proof of Concept

In

GenericBondCalculator, function calculatePayoutOLAS

    function calculatePayoutOLAS(uint256 tokenAmount, uint256 priceLP) external view
        returns (uint256 amountOLAS)
    {
        uint256 totalTokenValue = mulDiv(priceLP, tokenAmount, 1);

        // Check for the cumulative LP tokens value limit
        if (totalTokenValue > type(uint192).max) {
            revert Overflow(totalTokenValue, type(uint192).max);
        }

        amountOLAS = ITokenomics(tokenomics).getLastIDF() * totalTokenValue / 1e36;
    }

the amount of OLAS to be awarded in exchange of a given amount of another token follows the next formula (from the stripped comments):

$amountOLAS = IDF * priceLP * tokenAmount / 1e36$

if we expand a bit all the variables, we see that

$priceLP = 2 * reserves0 / liquidity * 1e18$

so, in order to calculate the intermediate totalTokenValue, the math should be:

$(priceLP * tokenAmount) / 1e18$

$totalTokenValue = ((2 * reserves0 / liquidity * 1e18) * tokenAmount) / 1e18$

$totalTokenValue = 2 * reserves0 / liquidity * tokenAmount$

However, if we see the highlighted code, it does not follow this due to a typo in the divisor being 1 instead of 1e18 to scalate priceLP. Because of that, totalTokenValue is 1e18 times higher than intended and the check

        // Check for the cumulative LP tokens value limit
        if (totalTokenValue > type(uint192).max) {
            revert Overflow(totalTokenValue, type(uint192).max);
        }

will apply a cap 1e18 times lower than intended, as the "to-be-calculated" amounts are not scaled correctly. This does not affect amountOLAS as it divided by 1e36, where it's included the 1e18 mentioned above.

Recommended Mitigation Steps

    function calculatePayoutOLAS(uint256 tokenAmount, uint256 priceLP) external view
        returns (uint256 amountOLAS)
    {
-       uint256 totalTokenValue = mulDiv(priceLP, tokenAmount, 1);
+       uint256 totalTokenValue = mulDiv(priceLP, tokenAmount, 1e18);

        // Check for the cumulative LP tokens value limit
        if (totalTokenValue > type(uint192).max) {
            revert Overflow(totalTokenValue, type(uint192).max);
        }

-       amountOLAS = ITokenomics(tokenomics).getLastIDF() * totalTokenValue / 1e36;
+       mountOLAS = ITokenomics(tokenomics).getLastIDF() * totalTokenValue / 1e18;
    }

Assessed type

Math