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

Should not minting new collection token before burning the old collection token #1198

Closed
c4-submissions opened this issue Nov 12, 2023 · 7 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 downgraded by judge Judge downgraded the risk level of this issue duplicate-1597 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L217-L222

Vulnerability details

Impact

Should not minting new collection token before burning the old collection token, or the old collection may be leveraged by the owner before burning.

Proof of Concept

mintAndAuction allows collection token owner to mint a new collection token by burning the old one. The minting and burning is done through burnToMint in NextGenCore contract.

    function burnToMint(uint256 mintIndex, uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o, address burner) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply = collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_mintCollectionID].collectionTotalSupply >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
            // burn token
            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
        }
    }

The new collection token will be minted to the receiver before the old collection token is burned.

Given that the old collection token can be used as a collateral in a lending platform as it has value of mintpass.
Consider a case where:

  1. Token owner calls mintAndAuction from a contract to mint a new collection token;
  2. New collection token is minted and transferred to the contract;
  3. onERC721Received is called on the contract, contract uses the old collection token as collateral and borrows money from the lending platform;
  4. Lending platform owns the old collection token but soon the token is burned by NextGenCore;
  5. User gets new collection token and money.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, please burn the old collection token before minting the new one.

Assessed type

ERC721

@c4-submissions c4-submissions added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 12, 2023
c4-submissions added a commit that referenced this issue Nov 12, 2023
@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Nov 19, 2023
@c4-pre-sort
Copy link

141345 marked the issue as primary issue

@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #1597

@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #1742

@c4-judge
Copy link

alex-ppg marked the issue as not a duplicate

@c4-judge
Copy link

alex-ppg marked the issue as duplicate of #1597

@c4-judge
Copy link

c4-judge commented Dec 5, 2023

alex-ppg changed the severity to 2 (Med Risk)

@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 downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Dec 5, 2023
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 8, 2023
@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as satisfactory

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 downgraded by judge Judge downgraded the risk level of this issue duplicate-1597 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants