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

deployHolographableContract() uniqueness #339

Closed
code423n4 opened this issue Oct 25, 2022 · 2 comments
Closed

deployHolographableContract() uniqueness #339

code423n4 opened this issue Oct 25, 2022 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) responded The Holograph team has reviewed and responded sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographFactory.sol#L206-L215

Vulnerability details

Impact

The holographable smart contract deployment can not guarantee the uniqueness. A malicious user could deploy multiple duplicate contracts, then the brigeIn could not make sure the uniqueness of the minted or burned NFT.

Proof of Concept

The salt and initCode could be changed and generate different hash.

// contracts/HolographFactory.sol
    bytes32 hash = keccak256(
      abi.encodePacked(
        config.contractType,
        config.chainType,
        config.salt,
        keccak256(config.byteCode),
        keccak256(config.initCode),
        signer
      )
    );

Tools Used

Manual analysis.

Recommended Mitigation Steps

Do not include salt and initCode in the hash.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Oct 25, 2022
code423n4 added a commit that referenced this issue Oct 25, 2022
@gzeoneth gzeoneth added the invalid This doesn't seem right label Oct 31, 2022
@gzeoneth
Copy link
Member

As you said, they would be different contract and whatever interact with them should verify the address.

@alexanderattar alexanderattar added sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue responded The Holograph team has reviewed and responded labels Nov 8, 2022
@alexanderattar
Copy link

Seconding the above

@alexanderattar alexanderattar added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) responded The Holograph team has reviewed and responded sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants