FragmentNFT Manual Review Findings

FNF-01M: Proposal Tag ID Corruption Re-Entrancy

Type	Severity	Location
Language Specific		FragmentNFT.sol:L293-L296

Description:

The FragmentNFT::proposeMany function contains a re-entrancy attack vector that manifests when the VerifierManager calls the FragmentNFT::accept function in the same call.

Specifically, the FragmentNFT::_proposeManyMessageHash function will validate a signed payload with a specific _mintCounter range, however, this range can be invalidated by re-entering the contract during an interim IVerifierManager::propose call and invoking f.e. DatasetNFT::proposeFragment, causing the _mintCounter to be incremented while the loop within FragmentNFT::proposeMany has not concluded and thus leading to out-of-order tag and pending fragment owner IDs.

Impact:

The current FragmentNFT::proposeMany mechanism is insecure as the signed message hash validated in FragmentNFT::_proposeManyMessageHash can be "invalidated" via a re-entrancy when the IVerifierManager instantly "accepts" proposals and thus leads to the ERC721::_safeMint operation being executed within FragmentNFT::accept which is re-entrant.

Example:

Copy
276function proposeMany(
277  address[] memory owners,
278  bytes32[] memory tags_,
279  bytes calldata signature
280) external onlyDatasetNFT {
281  if (tags_.length != owners.length) revert ARRAY_LENGTH_MISMATCH();
282  bytes32 msgHash = _proposeManyMessageHash(_mintCounter + 1, _mintCounter + tags_.length, owners, tags_);
283  address signer = ECDSA.recover(msgHash, signature);
284  if (!dataset.isSigner(signer)) revert BAD_SIGNATURE(msgHash, signer);
285
286  for (uint256 i; i < owners.length; i++) {
287    uint256 id = ++_mintCounter;
288    bytes32 tag = tags_[i];
289    pendingFragmentOwners[id] = owners[i];
290    tags[id] = tag;
291    emit FragmentPending(id, tag);
292
293    // Here we call VeriferManager and EXPECT it to call `accept()`
294    // during this call OR at any following transaction.
295    // DO NOT implement any state changes after this point!
296    IVerifierManager(dataset.verifierManager(datasetId)).propose(id, tag);
297  }
298}

Recommendation:

We advise the code to cache the _mintCounter to a local cachedMintCounter variable before the for loop's execution and to validate it afterwards as being strictly equal to the _mintCounter (i.e. cachedMintCounter + tags_.length == _mintCounter).

This mechanism will prevent re-entrancy attacks from affecting the sole storage related entry utilized within the vulnerable for loop within FragmentNFT::proposeMany.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

The code was updated according to our recommendation, caching the _mintCounter to a local lastMintCounter variable that is consequently utilized at the end of the function's body in a validation check ensuring that its post-loop state (_mintCounter) matches the value expected if all tags_ have been properly consumed and minted.

As such, we consider this exhibit fully alleviated.