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

Different position input of token0 and token1 , will result in different pool Id #497

Open
c4-bot-9 opened this issue Dec 11, 2023 · 5 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-07 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@c4-bot-9
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L48-L59

Vulnerability details

Impact

if change positions of token0 and token1 when calling getFinalPoolId, according the keccak256 function , will result same token pair to different pool id

Proof of Concept

    function getFinalPoolId(
        uint64 basePoolId,
        address token0,
        address token1,
        uint24 fee
    ) internal pure returns (uint64) {
        unchecked {
            return
                basePoolId +
                (uint64(uint256(keccak256(abi.encodePacked(token0, token1, fee)))) >> 32);
        }
    }

if we change token0 -> token1 and token1 -> token0, the pood id will be different .

Tools Used

Vscode

Recommended Mitigation Steps

use token address sort method

if (tokenA > tokenB) (tokenA, tokenB) = (tokenB, tokenA);

Assessed type

Library

@c4-bot-9 c4-bot-9 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 11, 2023
c4-bot-10 added a commit that referenced this issue Dec 11, 2023
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Dec 14, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as primary issue

@c4-sponsor
Copy link

dyedm1 (sponsor) disputed

@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Dec 18, 2023
@dyedm1
Copy link

dyedm1 commented Dec 18, 2023

This is true, but not an issue. At this point, the poolId is different than what can be computed from the address anyway, so you would have to call the getpoolid function regardless of the order the tokens are specified. It doesn't really matter whether the hash we use to differentiate the pools is calculated with one order of tokens or another - it actually makes it harder to make an undeployable pool since we have two different hash options.

@c4-judge
Copy link
Contributor

Picodes changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Dec 19, 2023
@c4-judge
Copy link
Contributor

Picodes marked the issue as grade-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-07 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

5 participants