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

There is a lack of checking the size of _tokmin and _tokmax #796

Closed
c4-submissions opened this issue Nov 10, 2023 · 5 comments
Closed

There is a lack of checking the size of _tokmin and _tokmax #796

c4-submissions opened this issue Nov 10, 2023 · 5 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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L315-L322

Vulnerability details

Impact

If the input _tokmax is smaller than _tokmin, the require statement in the burnOrSwapExternalToMint function, require(_tokenId >= burnOrSwapIds[externalCol][0] && _tokenId <= burnOrSwapIds[externalCol][1], "Token id does not match");, will never pass.

Proof of Concept

@ function initializeExternalBurnOrSwap(address _erc721Collection, uint256 _burnCollectionID, uint256 _mintCollectionID, uint256 _tokmin, uint256 _tokmax, address _burnOrSwapAddress, bool _status) public FunctionAdminRequired(this.initializeExternalBurnOrSwap.selector) {
bytes32 externalCol = keccak256(abi.encodePacked(_erc721Collection,_burnCollectionID));
require((gencore.retrievewereDataAdded(_mintCollectionID) == true), "No data");
burnExternalToMintCollections[externalCol][_mintCollectionID] = _status;
burnOrSwapAddress[externalCol] = _burnOrSwapAddress;
@ burnOrSwapIds[externalCol][0] = _tokmin;
@ burnOrSwapIds[externalCol][1] = _tokmax;
}
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L315-L322

function burnOrSwapExternalToMint(address _erc721Collection, uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, string memory _tokenData, bytes32[] calldata merkleProof, uint256 _saltfun_o) public payable {
    bytes32 externalCol = keccak256(abi.encodePacked(_erc721Collection,_burnCollectionID));
    require(burnExternalToMintCollections[externalCol][_mintCollectionID] == true, "Initialize external burn");
    require(setMintingCosts[_mintCollectionID] == true, "Set Minting Costs");
    address ownerOfToken = IERC721(_erc721Collection).ownerOf(_tokenId);
    if (msg.sender != ownerOfToken) {
        bool isAllowedToMint;
        isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(ownerOfToken, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(ownerOfToken, 0x8888888888888888888888888888888888888888, msg.sender, 2);
        if (isAllowedToMint == false) {
        isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(ownerOfToken, _erc721Collection, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(ownerOfToken, _erc721Collection, msg.sender, 2);    
        }
        require(isAllowedToMint == true, "No delegation");
    }

@ require(_tokenId >= burnOrSwapIds[externalCol][0] && _tokenId <= burnOrSwapIds[externalCol][1], "Token id does not match");
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L326C1-L339C132

Tools Used

Recommended Mitigation Steps

Add the corresponding check for this condition.

Assessed type

Context

@c4-submissions c4-submissions 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 Nov 10, 2023
c4-submissions added a commit that referenced this issue Nov 10, 2023
@c4-sponsor c4-sponsor added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Nov 23, 2023
@c4-sponsor
Copy link

a2rocket (sponsor) disputed

@a2rocket
Copy link

no need to add the check is unneccessary.

@c4-pre-sort
Copy link

141345 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Nov 27, 2023
@alex-ppg
Copy link

alex-ppg commented Dec 6, 2023

The Warden specifies that an invalid ID burn range can be specified for external burn-to-mint collections.

The range can be simply reconfigured, rendering this submission to be a QA (NC) and thus of overinflated severity.

@c4-judge c4-judge closed this as completed Dec 6, 2023
@c4-judge c4-judge added the unsatisfactory does not satisfy C4 submission criteria; not eligible for awards label Dec 6, 2023
@c4-judge
Copy link

c4-judge commented Dec 6, 2023

alex-ppg marked the issue as unsatisfactory:
Overinflated severity

This was referenced Dec 8, 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 sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Development

No branches or pull requests

6 participants