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

create methods are suspicious of the reorg attack #31

Open
code423n4 opened this issue Aug 5, 2023 · 10 comments · Fixed by GenerationSoftware/pt-v5-vault-boost#2
Open
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 high quality report This report is of especially high quality M-09 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBoosterFactory.sol#L29

Vulnerability details

Impact

The createVaultBooster() function deploys a new VaultBooster contract using the create, where the address derivation depends only on the VaultBoosterFactory nonce.

Re-orgs can happen in all EVM chains and as confirmed the contracts will be deployed on most EVM compatible L2s including Arbitrum, etc. It is also planned to be deployed on ZKSync in future. In ethereum, where this is deployed, Re-orgs has already been happened. For more info, check here

This issue will increase as some of the chains like Arbitrum and Polygon are suspicious of the reorg attacks.

Polygon re-org reference: click here This one happened this year in February, 2023.

Polygon blocks forked: check here

The issue would happen when users rely on the address derivation in advance or try to deploy the position clone with the same address on different EVM chains, any funds sent to the new contract could potentially be withdrawn by anyone else. All in all, it could lead to the theft of user funds.

File: src/VaultBoosterFactory.sol

    function createVaultBooster(PrizePool _prizePool, address _vault, address _owner) external returns (VaultBooster) {
>>        VaultBooster booster = new VaultBooster(_prizePool, _vault, _owner);

        emit CreatedVaultBooster(booster, _prizePool, _vault, _owner);

        return booster;
    }

Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation.

Attack Scenario
Imagine that Alice deploys a new VaultBooster, and then sends funds to it. Bob sees that the network block reorg happens and calls createVaultBooster. Thus, it creates VaultBooster with an address to which Alice sends funds. Then Alices’ transactions are executed and Alice transfers funds to Bob’s controlled VaultBooster.

This is a Medium severity issue that has been referenced from below Code4rena reports,
https://code4rena.com/reports/2023-01-rabbithole/#m-01-questfactory-is-suspicious-of-the-reorg-attack
https://code4rena.com/reports/2023-04-frankencoin#m-14-re-org-attack-in-factory

Proof of Concept

https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBoosterFactory.sol#L29

Tools Used

Manual Review

Recommended Mitigation Steps

Deploy such contracts via create2 with salt that includes msg.sender

Assessed type

Other

@code423n4 code423n4 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 Aug 5, 2023
code423n4 added a commit that referenced this issue Aug 5, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as duplicate of #169

@c4-pre-sort c4-pre-sort added duplicate-169 high quality report This report is of especially high quality labels Aug 7, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as high quality report

@c4-judge
Copy link
Contributor

HickupHH3 marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 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 Aug 12, 2023
@c4-judge
Copy link
Contributor

HickupHH3 changed the severity to QA (Quality Assurance)

@HickupHH3 HickupHH3 mentioned this issue Aug 15, 2023
@c4-judge
Copy link
Contributor

HickupHH3 marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Aug 15, 2023
@0xRizwan
Copy link

Hi @HickupHH3

I believe this is valid Medium and this report has more burden of proof than #169

While going through the discussion at #169 to know the reason of this issue being made invalid, i found your comment,

I shouldn't be doing the work to reason it out though, burden of proof lies with the wardens. None have sufficient justification to warrant the medium severity.

I think this reports has enough proofs for making the issue as valid.

In PoolTogether V5 audit which happended before this audit. This reorg issue has been made valid by Judge Picode and its very similar issue. Though the report is not public yet, I am putting below link from backstage for your reference,

code-423n4/2023-07-pooltogether-findings#416

PoolTogether V5 audit has made this issue to final report. I believe this report justifies the issue with references which can be seen in report.

Thank you!

@c4-judge c4-judge reopened this Aug 17, 2023
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 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 labels Aug 17, 2023
@c4-judge
Copy link
Contributor

This previously downgraded issue has been upgraded by HickupHH3

@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Aug 17, 2023
@c4-judge
Copy link
Contributor

HickupHH3 marked the issue as selected for report

@HickupHH3
Copy link

Accepting because of this:

Imagine that Alice deploys a new VaultBooster, and then sends funds to it. Bob sees that the network block reorg happens and calls createVaultBooster. Thus, it creates VaultBooster with an address to which Alice sends funds. Then Alices’ transactions are executed and Alice transfers funds to Bob’s controlled VaultBooster.

Again, the scenario should have been more explicit: stating how the vault could be different, how funds are transferred & possibly exploited.

@thebrittfactor thebrittfactor removed the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Aug 17, 2023
@thebrittfactor
Copy link

For transparency, the judge requested to remove the unsatisfactory label for this submission and any duplicates.

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 high quality report This report is of especially high quality M-09 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants