DatasetNFT Manual Review Findings

DNF-01M: Improper Enforcement Flow of Deployer Fees

Type	Severity	Location
Logical Fault		DatasetNFT.sol:L187-L199, L216-L219

Description:

The DatasetNFT::setDeployerFeeModelPercentages function can be invoked even when the contract has no deployerFeeBeneficiary specified. This can cause significant misbehaviours in the DistributionManager::receivePayment function as it will fatally fail if the deployerFeeBeneficiary has not been specified when a non-zero fee is expected to be applied.

Impact:

While the misconfiguration is trivial to resolve, it should be prohibited at the code level as otherwise valid subscriptions may fail to execute due to the DistributionManager::receivePayment function's failure, in turn leading to the subscription being lost as the subscriber may not wish to retry their operation.

Example:

Copy
180/**
181 * @notice Sets the fee percentages for the provided Deployer Fee Models
182 * @dev Only callable by DatasetNFT ADMIN.
183 * Percentages are encoded such that 100% is represented as 1e18.
184 * @param models An array of Deployer Fee Models to set percentages for (see `IDatasetNFT.sol`)
185 * @param percentages An array of corresponding fee percentages
186 */
187function setDeployerFeeModelPercentages(
188  DeployerFeeModel[] calldata models,
189  uint256[] calldata percentages
190) external onlyRole(DEFAULT_ADMIN_ROLE) {
191  if (models.length != percentages.length) revert ARRAY_LENGTH_MISMATCH();
192  for (uint256 i; i < models.length; i++) {
193    DeployerFeeModel m = models[i];
194    if (uint8(m) == 0) revert INVALID_ZERO_MODEL_FEE();
195    uint256 p = percentages[i];
196    if (p > 1e18) revert PERCENTAGE_VALUE_INVALID(1e18, p);
197    deployerFeeModelPercentage[m] = p;
198  }
199}
200
201/**
202 * @notice Sets the deployer fee model for a specific Dataset
203 * @dev Only callable by DatasetNFT ADMIN
204 * @param datasetId The ID of the target Dataset NFT token
205 * @param model The Deployer Fee Model to set
206 */
207function setDeployerFeeModel(uint256 datasetId, DeployerFeeModel model) external onlyRole(DEFAULT_ADMIN_ROLE) {
208  deployerFeeModels[datasetId] = model;
209}
210
211/**
212 * @notice Sets the address of the deployer fee beneficiary
213 * @dev Only callable by DatasetNFT ADMIN
214 * @param deployerFeeBeneficiary_  The address to set as the beneficiary
215 */
216function setDeployerFeeBeneficiary(address deployerFeeBeneficiary_) external onlyRole(DEFAULT_ADMIN_ROLE) {
217  if (deployerFeeBeneficiary_ == address(0)) revert ZERO_ADDRESS();
218  deployerFeeBeneficiary = deployerFeeBeneficiary_;
219}

Recommendation:

We advise the DatasetNFT::setDeployerFeeModelPercentages function to ensure that the deployerFeeBeneficiary has been specified to a non-zero value when it is invoked, preventing a non-zero fee from being applied to a particular model and data-set ID when no beneficiary for it has been specified.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

An if-revert pattern check was properly introduced ensuring that the deployerFeeBeneficiary has been specified when the DatasetNFT::setDeployerFeeModelPercentages function is invoked thus alleviating this exhibit in full.

DNF-02M: Inexistent Sanitization of Sensitive Module Implementations

Type	Severity	Location
Logical Fault		DatasetNFT.sol:L160-L178

Description:

The DatasetNFT::setManagers function permits the subscriptionManager, distributionManager, and verifierManager implementations that the system will utilize to be arbitrarily specified which is a significant security flaw in the system given that these modules are expected to utilize user funds, get approved by users to spend funds, and generally act as integral modules to the DatasetNFT contract's overall operation.

Impact:

Arbitrarily specified subscription and distribution implementations can lead to direct fund loss (by exploiting approvals) or to "free" data-set contributions (as the distributor could f.e. distribute all funds to a single party).

Example:

Copy
160function setManagers(uint256 id, ManagersConfig calldata config) external onlyTokenOwner(id) {
161  bool changed;
162  if (configurations[id].subscriptionManager != config.subscriptionManager) {
163    proxies[id].subscriptionManager = _cloneAndInitialize(config.subscriptionManager, id);
164    changed = true;
165  }
166  if (configurations[id].distributionManager != config.distributionManager) {
167    proxies[id].distributionManager = _cloneAndInitialize(config.distributionManager, id);
168    changed = true;
169  }
170  if (configurations[id].verifierManager != config.verifierManager) {
171    proxies[id].verifierManager = _cloneAndInitialize(config.verifierManager, id);
172    changed = true;
173  }
174  if (changed) {
175    configurations[id] = config;
176    emit ManagersConfigChange(id);
177  }
178}

Recommendation:

We advise the code to maintain an administrator-controlled implementation whitelist per manager type that the DatasetNFT::setManagers function sanitizes its inputs against, ensuring that the implementations attached to a particular data-set ID are secure and valid.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

A DatasetNFT::_checkManager function was introduced to the codebase that ensures the relevant implementations are non-zero, conform to a specific interface type, and have ultimately been whitelisted with the WHITELISTED_MANAGER_ROLE assigned via the AccessControlUpgradeable contract.

As a result of these changes, we consider this exhibit fully alleviated.

DNF-03M: Insecure Signature Validation

Type	Severity	Location
Logical Fault		DatasetNFT.sol:L141

Description:

The DatasetNFT::_mintMessageHash function will yield a message digest that does not contain the to member the mint operation will create an NFT to.

As such, all mint operations are susceptible to on-chain race conditions whereby a DatasetNFT::mint operation is detected and a malicious actor submits a transaction with a higher priority that changes the to argument to their choosing.

Impact:

The signature validation mechanism in DatasetNFT::mint is ineffectual as it does not validate the to member the NFT is being minted for, permitting anyone to claim any signature.

Example:

Copy
140function mint(bytes32 uuidHashed, address to, bytes calldata signature) external returns (uint256) {
141  bytes32 msgHash = _mintMessageHash(uuidHashed);
142  address signer = ECDSA.recover(msgHash, signature);
143  if (!hasRole(SIGNER_ROLE, signer)) revert BAD_SIGNATURE(msgHash, signer);
144
145  uint256 id = uint256(uuidHashed);
146
147  _mint(to, id);
148
149  return id;
150}

Recommendation:

We advise the code to properly attach the to member with the uuidHashed, preventing on-chain race conditions from being able to "steal" the mint operation of a particular data-set ID.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

The DatasetNFT::_mintMessageHash function was updated to properly include the to argument as part of the payload and the overall mint flow was additionally updated to be executed solely via the DatasetFactory contract and specifically its DatasetFactory::mintAndConfigureDataset function.

As a result of these changes, we consider the overall signature validation mechanism secure and the minting function of the DatasetNFT no longer prone to race conditions.