Delegatee will not be able to mint tokens on behalf of delegator during public phase

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

During the allowlist phase, a delegatee can mint tokens on behalf of the delegator through the mint() function. But during the public phase, the delegatee is unable to do so due to the NFTDelegation checking functionality being implemented only in the allowlist conditional block (as seen here).

This would prevent the delegators (who use cold wallets especially) from minting tokens. As we can see in the 2nd point here from the NFTDelegation documentation provided by the team, users use delegation assignments if they do not want to risk connecting and signing messages from a cold wallet.

Proof of Concept

In the mint() function below, Lines 202 to 220 represent the conditional block for the allowlistPhase while Lines 221 to 226 represent the conditional block for the public phase.

File: MinterContract.sol
202:         if (block.timestamp >= collectionPhases[col].allowlistStartTime && block.timestamp <= collectionPhases[col].allowlistEndTime) {
203:             phase = 1;
204:             bytes32 node;
205:             if (_delegator != 0x0000000000000000000000000000000000000000) {
206:                 bool isAllowedToMint;
207:                 isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 2);
208:                 if (isAllowedToMint == false) {
209:                 isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 2);    
210:                 }
211:                 require(isAllowedToMint == true, "No delegation");
212:                 node = keccak256(abi.encodePacked(_delegator, _maxAllowance, tokData));
213:                 require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");
214:                 mintingAddress = _delegator;
215:             } else {
216:                 node = keccak256(abi.encodePacked(msg.sender, _maxAllowance, tokData));
217:                 require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
218:                 mintingAddress = msg.sender;
219:             }
220:             require(MerkleProof.verifyCalldata(merkleProof, collectionPhases[col].merkleRoot, node), 'invalid proof');
221:         } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
222:             phase = 2;
223:             require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
224:             require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
225:             mintingAddress = msg.sender;
226:             tokData = '"public"';

2. In the above code, we can clearly see that Lines 206 to 214 allow a delegatee to mint on behalf of the delegator (Note: Owner of the NFT will be the delegator - see Line 214) during the allowlist phase but if we observe Lines 221 to 225, there is no implementation (as done in the allowlistPhase block above) to allow msg.sender (delegatee) to mint on behalf of delegator.

3. If the delegatee decides to call the mint() function during the public phase, the minting address is set to msg.sender (the delegatee himself - check Line 225 below) and thus the NFT is minted to the delegatee. As I mentioned above with the reference to the NFTDelegation documentation, delegators use delegation assignments if they do not want to risk connecting and signing messages from a cold wallet. Thus, such an implementation would block them from minting tokens on any NextGen collection during the public phase.

File: MinterContract.sol
221:         } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
222:             phase = 2;
223:             require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
224:             require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
225:             mintingAddress = msg.sender;
226:             tokData = '"public"';

Tools Used

Manual Review

Recommended Mitigation Steps

Similar to the burnOrSwapExternalToMint(), consider moving the NFTDelegation checking functionality outside the allowlist block to ensure the functionality exists during both the allowlistPhase and public phase.

It would make sense to duplicate the allowlist implementation in the public phase conditional block as well but that solution would cost more gas.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as duplicate of #1933

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as primary issue

[alex-ppg]

The Warden specifies that delegation-based mint operations can solely occur during the allowlist phase and not during the public phase.

The current delegation system will also mint the NFT acquired to the delegator, meaning that it makes sense for it to remain a feature of the public phase.

I do not consider this to be a medium-severity finding and consider it to be an Analysis or QA (NC) finding given that the documentation of NextGen explicitly states that delegations are meant to be used in allowlist phases.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity

[mcgrathcoutinho]

Hi @alex-ppg , here are some points as to why I believe this issue is a valid medium.

1. We can clearly see that the issue mentioned here is inconsistent with the burnOrSwapExternalToMint() that implements the delegation mechanism outside the allowlist conditional block.
2. The NextGen documentation states that delegations are meant to be used during minting (as shown here in use case 2). This is not specific to the allowlist phase only. We can see that this is proved by the presence of the delegation mechanism outside the allowlist conditional block in burnOrSwapExternalToMint(), which means it works for the public phase as well.
3. This is a clear inconsistency and thus I believe is deserving medium severity since the protocol does not work as intended.

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for following up on this exhibit! I will retain my original judgment as it is entirely up to preference whether delegation should be supported in public mints.

No form of loss or vulnerability arises from this. It is a missing feature that is never advertised as existing in the documentation of NextGen. As such, it is better suited as an Analysis or QA (NC) submission per my original verdict.

[mcgrathcoutinho]

Hi @alex-ppg, thanks for the response.

A delegatee (assigned by a delegator) is not always a hot wallet and can be a intermediate/router contract as well. This would mean tokens going to that external contract, which might not have a withdrawal mechanism.

If we take a scenario where a collection uses:

1. burnOrSwapExternalToMint() for the first public phase.
2. Following which, normal minting through mint() for the second public phase.

If the user uses delegation mechanism during the first phase and sees it works with burnOrSwapExternalToMint(), the user is bound to use it during the second phase through mint() as well. This would result in sending the NFT to the intermediate contract and thus can become unwithdrawable. This could also be seen as an issue arising because there isn't enough information in the docs, which means a higher chance users could accept the delegation mechanism being true in the second phase as well.

Although this is not advertised in the documentation anywhere, in the above scenario this inconsistency would definitely be harmful to the users.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for providing a potential scenario to accompany your comments! When we start constructing scenarios not specified in the documentation, we are delving deep into the realm of hypotheticals. In this particular case, you are describing a situation whereby the MinterContract is misused given that it does not advertise support for delegation in its public phase.

This particular scenario would fall under vulnerabilities conditional on a user mistake which are considered QA or invalid per the relevant SC verdict. A submission that relies on an external user incorrectly integrating the contract because they did not follow its documentation is OOS and should be reported in that contract's audit, not this one's.