createPoolD650E2D0 will not work due to mismatch in solidity and stylus function definitions

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/sol/SeawaterAMM.sol#L160-L168
https://github.com/code-423n4/2024-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/seawater/src/lib.rs#L802

Vulnerability details

Proof of Concept

1. Context

SeaWaterAMM.sol holds the createPoolD650E2D0 which allows the admin to initialize a new pool. It calls the create_pool_D650_E2_D0 function in the stylus.

2. Bug Location

As can be seen from the create_pool_D650_E2_D0 function, it takes in the token address, sqrtPriceX96 and fee.

    pub fn create_pool_D650_E2_D0(
        &mut self,
        pool: Address,
        price: U256,
        fee: u32,
    ) -> Result<(), Revert> {
 //...
     }
 }

But createPoolD650E2D0's definition takens in more, token address, sqrtPriceX96, fee, tick spacing and maxLiquidityPerTick, causing a mismatch between the function definitions of the Solidity and Stylus contracts.

    function createPoolD650E2D0( //@audit
        address /* token */,
        uint256 /* sqrtPriceX96 */,
        uint32 /* fee */,
        uint8 /* tickSpacing */,
        uint128 /* maxLiquidityPerTick */
    ) external {
        directDelegate(_getExecutorAdmin());
    }

3. Final Effect

Calls to the function will always fail, breaking SeawaterAMM.sol's functionality to create a pool position.

Recommended Mitigation Steps

Remove the unneeded parameters.

    function createPoolD650E2D0( //@audit
        address /* token */,
        uint256 /* sqrtPriceX96 */,
        uint32 /* fee */,
-       uint8 /* tickSpacing */,
-       uint128 /* maxLiquidityPerTick */
    ) external {
        directDelegate(_getExecutorAdmin());
    }

Assessed type

Context

[alex-ppg]

The Warden has correctly identified that the function definitions of the Solidity and Stylus contracts differ, resulting in the relevant functionality of the system being inaccessible.

In line with the previous contest's rulings, I believe a high-risk rating is appropriate for this submission as pool creations are rendered inaccessible via any other functions in contrast to the original contest's submission which permitted circumvention of this error.

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[DadeKuma]

Dear Judge,

I believe some key pieces of information are missing to provide an accurate severity assessment, which I will address in this comment.

It is true that createPoolD650E2D0 will not work if directly called, as it has the wrong ABI, and this finding is technically valid. However, there is a fallback function that allows the creation of new pools by using the correct ABI.

The correct ABI, like this issue points, is the following:

createPoolD650E2D0(address,uint256,uint32)

So the third byte is 0x80:

function testAbi() public pure returns (bytes1) {
    return abi.encodeWithSignature("createPoolD650E2D0(address,uint256,uint32)", address(0), 0, 0)[2];
}

decoded output {
"0": "bytes1: 0x80"
}

If we look at the fallback function, the execution will fall under the executor fallback:

fallback() external {
    // swaps
    if (uint8(msg.data[2]) == EXECUTOR_SWAP_DISPATCH)
        directDelegate(_getExecutorSwap());
        // update positions
    else if (uint8(msg.data[2]) == EXECUTOR_UPDATE_POSITION_DISPATCH)
        directDelegate(_getExecutorUpdatePosition());
        // positions
    else if (uint8(msg.data[2]) == EXECUTOR_POSITION_DISPATCH)
        directDelegate(_getExecutorPosition());
        // admin
    else if (uint8(msg.data[2]) == EXECUTOR_ADMIN_DISPATCH)
        directDelegate(_getExecutorAdmin());
        // swap permit 2
    else if (uint8(msg.data[2]) == EXECUTOR_SWAP_PERMIT2_A_DISPATCH)
        directDelegate(_getExecutorSwapPermit2A());
        // quotes
    else if (uint8(msg.data[2]) == EXECUTOR_QUOTES_DISPATCH) directDelegate(_getExecutorQuote());
    else if (uint8(msg.data[2]) == EXECUTOR_ADJUST_POSITION_DISPATCH) directDelegate(_getExecutorAdjustPosition());
    else if (uint8(msg.data[2]) == EXECUTOR_SWAP_PERMIT2_B_DISPATCH) directDelegate(_getExecutorSwapPermit2B());
->  else directDelegate(_getExecutorFallback());
}

https://github.com/code-423n4/2024-10-superposition/blob/7ad51104a8514d46e5c3d756264564426f2927fe/pkg/sol/SeawaterAMM.sol#L505

Current values:

uint8 constant EXECUTOR_SWAP_DISPATCH = 0;
uint8 constant EXECUTOR_UPDATE_POSITION_DISPATCH = 1;
uint8 constant EXECUTOR_POSITION_DISPATCH = 2;
uint8 constant EXECUTOR_ADMIN_DISPATCH = 3;
uint8 constant EXECUTOR_SWAP_PERMIT2_A_DISPATCH = 4;
uint8 constant EXECUTOR_QUOTES_DISPATCH = 5;
uint8 constant EXECUTOR_ADJUST_POSITION_DISPATCH = 6;
uint8 constant EXECUTOR_SWAP_PERMIT2_B_DISPATCH = 7;

Moreover, creating a pool is permissionless and intended by the Sponsor, it doesn't have to be called by the executor admin; the executor fallback would be able to create new pools.

Therefore, there is no loss of funds, and the functionality of the protocol is not impacted in this way; I don't see how an H risk can be justified. I believe this issue falls under the QA umbrella, as a function does not work according to specifications.

[alex-ppg]

Hey @DadeKuma, thanks for your PJQA contribution! This submission's assessment is in line with the previous contest and the presence of a fallback mechanism is not sufficient to justify the finding's invalidation. Otherwise, any inaccessible functionality of the system could be argued as being present in the fallback function and all findings pertaining to it would have to be invalidated in the previous contest as well.

Given that wardens were aware of the judgment style of this particular submission type, I do not believe that downgrading it in the follow-up round is a fair approach.

[DadeKuma]

@alex-ppg Thank you for your explanation. Based on this logic, I have a similar finding that I believe should not have been invalidated in the validation repo:

code-423n4/2024-10-superposition-validation#21

Could you take a look to ensure the judgment remains consistent?

EDIT: Moreover, given the alternative pathway through the fallback function, I believe this is M risk rather than H, similar to the previous finding in the last contest.

[alex-ppg]

Hey @DadeKuma, thanks for bringing this to my attention! Validation repository findings aren't directly evaluated by judges, so it is helpful to bring them to light. The medium severity rating on the original submission was assigned due to the impact of the missing functionality rather than the issue itself.

I will proceed with moving the relevant linked finding from the validation repository and assigning a medium-risk rating in line with the original.