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

Use of _mint in ReraiseETHCrowdfund#_contribute is incompatible with PartyGovernanceNFT#mint #42

Open
code423n4 opened this issue Apr 14, 2023 · 3 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 M-01 satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-04-party/blob/440aafacb0f15d037594cebc85fd471729bcb6d9/contracts/crowdfund/ReraiseETHCrowdfund.sol#L256-L303

Vulnerability details

Impact

Misconfigured receiver could accidentally DOS party

Proof of Concept

ReraiseETHCrowdfund.sol#L238

    if (previousVotingPower == 0) _mint(contributor); <- @audit-issue standard minting here

ReraiseETHCrowdfund.sol#L374

        uint256 tokenId = party.mint(contributor, votingPowerByCard[i], delegate); <- @audit-issue uses party.mint

PartyGovernanceNFT.sol#L162

    _safeMint(owner, tokenId); <- @audit-issue PartyGovernanceNFT#mint utilizes _safeMint

The issue at hand is that ReraiseETHCrowdfund#_contribute and PartyGovernanceNFT#mint use inconsistent minting methods. PartyGovernanceNFT uses safeMint whereas ReraiseETHCrowdfund uses the standard mint. This is problematic because this means that a contract that doesn't implement ERC721Receiver can receive a CrowdfundNFT but they can never claim because safeMint will always revert. This can cause a party to be inadvertently DOS'd because CrowdfundNFTs are soul bound and can't be transferred

Tools Used

Manual Review

Recommended Mitigation Steps

Use _safeMint instead of _mint for ReraiseETHCrowdfund#_contribute

@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 Apr 14, 2023
code423n4 added a commit that referenced this issue Apr 14, 2023
@0xble
Copy link

0xble commented Apr 17, 2023

I think a better mitigation would be to allow the user to specify a receiver address that can receive the party NFT when claiming, so if they cannot claim themselves they can specify another address that should receive it instead. It works similarly in Crowdfund, used to implement prior crowdfunds.

@c4-sponsor
Copy link

0xble marked the issue as sponsor confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Apr 17, 2023
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Apr 25, 2023
@c4-judge
Copy link

0xean marked the issue as satisfactory

@C4-Staff C4-Staff added selected for report This submission will be included/highlighted in the audit report M-01 labels Apr 28, 2023
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 M-01 satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report 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