minting doesn't follow chainlink security recommendations

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L229-L231 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L299-L303

Vulnerability details

Impact

According to chainlink docs:

Consider the example of a contract that mints a random NFT in response to a user's actions. The contract should:

1. Record whatever actions of the user may affect the generated NFT.
2. Stop accepting further user actions that might affect the generated NFT and issue a randomness request.
3. On randomness fulfillment, mint the NFT.

and

the cryptoeconomic security properties may be violated by an attacker that can rewrite the chain.

This is not followed by the protocol and thus doesn't conform to chainlink best practices. ARRNG doesn't have as thorough documentation but given that their architecture and flow is largely the same as chainlink the same would apply.

Proof of Concept

When minting an NFT a request for randomness is sent:

NextGenCore::_mintProcessing:

File: smart-contracts/NextGenCore.sol

229:        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
230:        tokenIdsToCollectionIds[_mintIndex] = _collectionID;
231:        _safeMint(_recipient, _mintIndex);

Then whenever the randomness is fulfulled this is passed back to the contract in NextGenCore::setTokenHash:

File: smart-contracts/NextGenCore.sol

299:    function setTokenHash(uint256 _collectionID, uint256 _mintIndex, bytes32 _hash) external {
300:        require(msg.sender == collectionAdditionalData[_collectionID].randomizerContract);
301:        require(tokenToHash[_mintIndex] == 0x0000000000000000000000000000000000000000000000000000000000000000);
302:        tokenToHash[_mintIndex] = _hash;
303:    }

This is against what chainlink best practices suggest.

Tools Used

Manual review and chainlink docs

Recommended Mitigation Steps

Consider changing the architecture so that the token is minted when randomness is received. This would still be instant for NXT randomness but introduce a delay for VRF and ARRNG.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

this is the intended design, the token is minted and has a pending metadata, once the hash is returned the metadata appear.

[alex-ppg]

The Warden specifies that the Randomizer implementation for the ARRNG and Chainlink services does not follow the latter's recommended security practices.

The Sponsor specifies that this is intended behavior, however, I agree with the Warden in that the security recommendations by Chainlink should be adhered to.

The impact of this exhibit is low as it would solely affect secondary sales of the NFT conditional on user mistake as the user would see no artwork/hash has been attached to the collection. The buyer of the NFT will already have supplied the necessary funds to purchase the NFT and if an auction is made with a token that is pending a hash's configuration, bidders who no longer want the NFT after its hash has been set can simply cancel their bids (and should not bid in the first place).

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)