Olas - BugzyVonBuggernaut's results

Lines of code

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

Vulnerability details

Impact

liquidity_lockbox#withdraw is at high risk of a permanent DOS due to "0" amount liquidity positions

Proof of Concept

liquidity_lockbox.sol#L95-L97

if (positionData.liquidity > type(uint64).max) {
    revert("Liquidity overflow");
}

There is no minimum amount liquidity check during LP deposits.

liquidity_lockbox.sol#L223-L225

    function withdraw(uint64 amount) external {
        address positionAddress = positionAccounts[firstAvailablePositionAccountIndex];
        if (positionAddress != tx.accounts.position.key) {
            revert("Wrong liquidity token account");
        }

        address pdaPositionAta = tx.accounts.pdaPositionAccount.key;
        if (mapPositionAccountPdaAta[positionAddress] != pdaPositionAta) {
            revert("Wrong position ATA");
        }

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

However, if the next available firstAvailablePositionAccountIndex has zero positionLiquidity, the position will revert.

liquidity_lockbox.sol#L321

firstAvailablePositionAccountIndex++;

The index only increments at the end of a successful withdraw call.

whirlpool-impl.ts#L268

invariant(liquidity.gt(new BN(0)), "liquidity must be greater than zero");

When depositing and opening a position via the Orca SDK, there is a check to ensure that the liquidity deposited is greater than 0.

However, this check is part of the SDK, not the on-chain code.

lib.rs#L160-L182

/// Open a position in a Whirlpool. A unique token will be minted to represent the position
/// in the users wallet. The position will start off with 0 liquidity.

So long as the SDK is bypassed, a 0 liquidity position can be open.

Due to the existing CPI issues between Autonolas and Orca, it's very difficult to prove the DOS directly. However, it is possible to demonstrate how to open a 0 liquidity position on Orca by removing the frontend invariant check and running their test suite.

For that please refer to this gist. Note that due to the changes, various tests will fail, but the one we are interested in that does pass is: open and add liquidity to a position, then close

Tools Used

VSC, Anchor

Recommended Mitigation Steps

The required step is to include a minimum liquidity constraint on Lockbox deposits.

An optional second step is to address the 0 position liquidity check in withdraw.

if (positionLiquidity == 0) {
	revert("No liquidity on a provided token account");
}

One option is to remove it entirely, or remove the revert and instead increment the index and exit the function.

if (positionLiquidity == 0) {
	firstAvailablePositionAccountIndex++
	return
}

This would remove the risk of a 0 amount DOS in the event that it somehow occurs despite the extra check on deposits.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Withdrawing non-programmatically from the lockbox becomes impossible in practice due to an attacker creating many small positions.

This is due to two existing conditions: a) No deposit lower bound b) Withdrawals happen in strict FIFO order.

Proof of Concept

liquidity_lockbox.sol#L210-L211

function withdraw(uint64 amount) external {
        address positionAddress = positionAccounts[firstAvailablePositionAccountIndex];

As outlined in the docs, lockbox withdrawals "checks whether the liquidity L associated with the least recently deposited NFT is larger than or equals to the requested withdrawal amount X. If this condition is met, the corresponding liquidity is liquidated, and the X amount of fungible tokens is burnt"

Importantly, "multiple withdrawal calls may be necessary, contingent on whether the user's fungible token amount, X, is greater than the least recently deposited NFT".

liquidity_lockbox.sol#L320-L321

// Increase the first available position account index
firstAvailablePositionAccountIndex++;

At the end of withdraw, the index is incremented.

liquidity_lockbox.sol#L140

There is no minimum deposit amount in liquidity_lockbox::deposit.

liquidity_lockbox.ts#L48

- const usdc_amount = DecimalUtil.toBN(new Decimal("10" /* usdc */), 6);
+ const usdc_amount = DecimalUtil.toBN(new Decimal("1" /* usdc */), 0);

Run the tests with the above adjustment to verify.

An attacker can grief lockbox users by creating thousands of small LP positions and forcing users to execute withdraw thousands of times in order to get their LP tokens back.

Tools Used

VSC

Recommended Mitigation Steps

Include a minimum deposit amount in the lockbox and consider allowing users to withdraw from specific positions, or consider reordering the LP positions from large to small.

The latter two would circumvent the potential issue where even if there were many reasonable minimum deposits (1000 USD), a user dealing in much more size (1,000,000 USD) wouldn't have to call withdraw 1000 times or more.

Assessed type

Other