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

Nondeterministic clone can cause issues in case of reorg #573

Open
c4-bot-1 opened this issue Apr 22, 2024 · 6 comments
Open

Nondeterministic clone can cause issues in case of reorg #573

c4-bot-1 opened this issue Apr 22, 2024 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-a primary issue Highest quality submission among a set of duplicates Q-02 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-1
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticFactory.sol#L240-L245

Vulnerability details

Impact

Collateral trackers are cloned non-deterministically. In case of a reorg this can cause users to deposit different amounts of funds into trackers than they intended to and leaving them with a lower liquidation point than planned.

Proof of Concept

The PanopticFactory.deployNewPool function creates new pools and contracts like this:

newPoolContract = PanopticPool(POOL_REFERENCE.cloneDeterministic(salt));

CollateralTracker collateralTracker0 = CollateralTracker(
    Clones.clone(COLLATERAL_REFERENCE)
);
CollateralTracker collateralTracker1 = CollateralTracker(
    Clones.clone(COLLATERAL_REFERENCE)
);

While the pool address is deterministic, the collateral tracker addresses are not. A scenario where this can cause issues:

  • Block 1: 2 panoptic pools for 2 different uniswap pools are deployed
    • PanopticPool-1 has address 0x123
      • CollateralTracker (PanopticPool1-token0) has address 0x1
      • CollateralTracker (PanopticPool1-token1) has address 0x2
    • PanopticPool-2 has address 0x456
      • CollateralTracker (PanopticPool1-token0) has address 0x3
      • CollateralTracker (PanopticPool1-token1) has address 0x4
  • Block 2: a user operates a bot that automatically deposits funds and sells options if deemed potentially lucrative
    • he deposits a lot of collateral into the PanopticPool-1 collateral trackers and sells options (intention is to have a liquidation threshold higher than the strike price)
    • he deposits less collateral into the PanopticPool-2 collateral trackers and sells options

Now imagine a reorg happens and the new Block 1 will have swapped the deployment transactions:

  • PanopticPool-2 still has address 0x456
    • CollateralTracker (PanopticPool2-token0) has address 0x1 (previously 0x3)
    • CollateralTracker (PanopticPool2-token1) has address 0x2 (previously 0x4)
  • PanopticPool-1 still has address 0x123
    • CollateralTracker (PanopticPool1-token0) has address 0x3 (previously 0x1)
    • CollateralTracker (PanopticPool1-token1) has address 0x4 (previously 0x2)

Since the transactions of the user bot have already been submitted they will still be processed with their original tx data. In a good case, they will just revert, but it can also happen that they pass and the user will have deposited collateral and sold options, except now the collateral deposits have been reversed and the user may end up with a lower liquidation point for a more volatile pool.

Tools Used

Manual review

Recommended Mitigation Steps

Also use cloneDeterministic for the CollateralTrackers (can use the same salt as used for the PanopticPool)

Assessed type

Other

@c4-bot-1 c4-bot-1 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 Apr 22, 2024
c4-bot-1 added a commit that referenced this issue Apr 22, 2024
@Picodes
Copy link

Picodes commented Apr 25, 2024

At first sight, this is a very very extreme scenario. You need the same underlying tokens, correct approvals, etc

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Apr 25, 2024
@c4-judge
Copy link
Contributor

Picodes marked the issue as primary issue

@dyedm1 dyedm1 added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Apr 26, 2024
@dyedm1
Copy link
Member

dyedm1 commented Apr 26, 2024

Technically possible, but extremely unlikely (especially if a deliberate attempt was made to create a malicious pool, which is the only way this could cause any real issues). Not sure if warrants medium severity given the very low probability.

@Picodes
Copy link

Picodes commented Apr 29, 2024

I don't think the described scenario is worth Medium severity. It would require a gigantic amount of bad luck. However, it's possible that following this reorg idea and the sponsor's suggestion we could find a Medium severity scenario, for example, if an attacker was spamming deployments on malicious pools hoping at some point a collateral deposit would happen on their malicious pools due to a reorg.

@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 Apr 29, 2024
@c4-judge
Copy link
Contributor

Picodes changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

Picodes marked the issue as grade-a

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-a primary issue Highest quality submission among a set of duplicates Q-02 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

5 participants