AcceptManuallyVerifier Manual Review Findings

AMV-01M: Insecure Utilization of Fragment NFT

Type	Severity	Location
Logical Fault		AcceptManuallyVerifier.sol:L28-L30, L36-L38, L73-L75, L77, L91-L93, L95, L108-L110, L114

Description:

All referenced statements utilize the fragmentNFT the caller specifies and utilize its getter functions to assess the address at which an IVerifierManager::resolve call should be performed at.

This evaluation path is insecure as it is possible for a user to submit a malicious fragmentNFT that passes the relevant access-control checks for the AcceptManuallyVerifier::onlyVerifierManager / AcceptManuallyVerifier::onlyDatasetOwner and ultimately yields a valid IVerifierManager where the VerifierManager::resolve call is made to.

As the VerifierManager::resolve function will evaluate the IFragmentNFT to perform a call to via the dataset and datasetId entries it contains, a malicious contract can be utilized to trick a "manual" acceptance of a fragment ID.

Impact:

THe AcceptManuallyVerifier can be completely bypassed by a carefully crafted IFragmentNFT and IDatasetNFT compliant malicious contract.

Example:

Copy
82/**
83 * @notice Resolves a single contribution proposal
84 * @dev Only callable by the Dataset owner.
85 * Emits a {FragmentResolved} event.
86 * @param fragmentNFT The address of the FragmentNFT contract instance
87 * @param id The ID of the pending Fragment associated with the contribution proposal
88 * @param accept Flag to indicate acceptance (`true`) or rejection (`true`)
89 */
90function resolve(address fragmentNFT, uint256 id, bool accept) external onlyDatasetOwner(fragmentNFT) {
91  VerifierManager vm = VerifierManager(
92    IDatasetNFT(IFragmentNFT(fragmentNFT).dataset()).verifierManager(IFragmentNFT(fragmentNFT).datasetId())
93  );
94  _pendingFragments[fragmentNFT].remove(id);
95  vm.resolve(id, accept);
96  emit FragmentResolved(fragmentNFT, id, accept);
97}

Recommendation:

We advise the code to re-evaluate its approach, configuring the relevant addresses it is meant to invoke in an AcceptManuallyVerifier::constructor, ensuring that the input fragmentNFT is validated as a correct implementation by querying the IDatasetNFT::fragmentNFT getter function with the relevant datasetId.

Alleviation (fb50b5c39665f7df086b2de1fdbf93ba2d836bf9):

The AcceptManuallyVerifier contract was updated to implement a constructor that specifies the dataset the verifier is valid for. All relevant statements have thus been updated to use the contract-configured dataset value rather than the fragmentNFT yielded one, permitting proper validation of the input fragmentNFT and relevant configuration by correctly utilizing the constructor specified dataset contract.