Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hardcoded Pool Selection in ZetaTokenConsumerTrident Contract #152

Closed
c4-bot-8 opened this issue Dec 1, 2023 · 7 comments
Closed

Hardcoded Pool Selection in ZetaTokenConsumerTrident Contract #152

c4-bot-8 opened this issue Dec 1, 2023 · 7 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-bot-8
Copy link
Contributor

c4-bot-8 commented Dec 1, 2023

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerTrident.strategy.sol#L88
https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/protocol-contracts/contracts/evm/tools/ZetaTokenConsumerTrident.strategy.sol#L154

Vulnerability details

Impact

  • Suboptimal Swaps: The contract employs a hardcoded pool selection mechanism that always chooses the first pool (pairPools[0]) when executing swaps. This rigid approach may lead to suboptimal swap rates and increased transaction costs for users. Suboptimal swaps result in users receiving fewer tokens than they should for the same input, significantly impacting their trading experience. Users may suffer financial losses due to the inefficiencies in the contract's token swap mechanism.

Proof of Concept

Consider the following:

  • Alice calls the getZetaFromEth function providing the destination address and the minAmountOut she wants
  • When performing the swap the function getZetaFromEth is getting ths list of pool by using this line
        address[] memory pairPools = poolFactory.getPools(token0, token1, 0, 1);
  • The function will then always using the pool[0] when performing the swap.
  • There could be multiple pool path for the swap, but the function is always choosing the first pool for the swap.

As the function is consistently selecting the first pool (pairPools[0]), it may not opt for the most liquid pool available. Consequently, this results in suboptimal swap rates for users, causing them to receive fewer ZetaTokens than they should have received for the same amount of ETH input.

Tools Used

Manual Review

Recommended Mitigation Steps

To address and mitigate the identified vulnerability, it is essential to make the contract's pool selection process dynamic, allowing it to choose the pool with the highest liquidity for each swap. Here are the recommended mitigation steps:

Dynamic Pool Selection: Modify the getZetaFromEth function to dynamically select the pool with the highest liquidity for the intended swap. Achieve this by iterating through the available pools and evaluating their liquidity.

// Modify the code snippet in getZetaFromEth function
uint256 maxLiquidity = 0;
address selectedPool;

for (uint256 i = 0; i < pairPools.length; i++) {
    uint256 liquidity = // Calculate the liquidity of the pool.

    if (liquidity > maxLiquidity) {
        maxLiquidity = liquidity;
        selectedPool = pairPools[i];
    }
}

IPoolRouter.ExactInputSingleParams memory params = IPoolRouter.ExactInputSingleParams({
    tokenIn: address(0),
    amountIn: msg.value,
    amountOutMinimum: minAmountOut,
    pool: selectedPool, // Use the selected pool with the highest liquidity
    to: destinationAddress,
    unwrap: false
});

Assessed type

Error

@c4-bot-8 c4-bot-8 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 1, 2023
c4-bot-8 added a commit that referenced this issue Dec 1, 2023
@DadeKuma
Copy link

QA might be more appropriate

@c4-pre-sort
Copy link

DadeKuma marked the issue as insufficient quality report

@c4-pre-sort c4-pre-sort added the insufficient quality report This report is not of sufficient quality label Dec 20, 2023
@c4-judge c4-judge closed this as completed Jan 7, 2024
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Jan 7, 2024
@c4-judge
Copy link

c4-judge commented Jan 7, 2024

0xean marked the issue as unsatisfactory:
Overinflated severity

@Nabeel-javaid
Copy link

Hey, Really appreciate your review but I disagree with the decision.

https://solodit.xyz/issues/m-08-_swapuniswapv2-may-use-an-improper-path-which-can-cause-a-loss-of-the-majority-of-the-rewardtokens-code4rena-jpegd-jpegd-contest-git

https://solodit.xyz/issues/m-02-hardcoded-swap-path-might-not-be-the-most-optimalliquid-one-pashov-none-yield-ninja-markdown_

Above are the two issues that are very similar to what I've submitted. Due to the hard coding of Pool[0] if the pool is not the most liquid one then due to slippage user will receive very less Tokens but as the minOut is specified so the swap will revert cause DOS

@DadeKuma
Copy link

There isn't a loss of funds as there is slippage protection and users expect to get at least minAmountOut.

@0xean
Copy link

0xean commented Jan 12, 2024

leaving as marked. This is equivalent to saying the contract should actually calculate the best multi transaction route also because a user would get a better rate. While true, it doesn't mean it qualifies as M

@0xean
Copy link

0xean commented Jan 12, 2024

(final decision here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working insufficient quality report This report is not of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants