CollectionWideOfferVerifier Manual Review Findings

CWO-01M: Unsafe Casting Operation

Type	Severity	Location
Mathematical Operations		CollectionWideOfferVerifier.sol:L57

Description:

The CollectionWideOfferVerifier::verifyPredicates function will improperly cast the input collateralId to a uint160 value without ensuring that the value lies within its bounds, leading to a casting overflow.

Impact:

It is currently possible for a CollectionWideOfferVerifier::verifyPredicates operation to succeed incorrectly as it will downcast a uint256 value to uint160, causing multiple different collateralId values to result in the same vaultAddress and thus bypass the contract's verification.

Example:

Copy
44function verifyPredicates(
45    address, address,
46    address collateralAddress,
47    uint256 collateralId,
48    bytes calldata data
49) external view override returns (bool) {
50    // Unpack items
51    (address token) = abi.decode(data, (address));
52
53    // Unvaulted case - collateral will be escrowed directly
54    if (collateralAddress == token) return true;
55
56    // Do vault check
57    address vaultAddress = address(uint160(collateralId));
58
59    if (!IVaultFactory(collateralAddress).isInstance(vaultAddress)) return false;
60
61    return IERC721(token).balanceOf(vaultAddress) > 0;
62}

Recommendation:

Given that casting overflows are not automatically detected using checked arithmetic, we advise the code to utilize a library such as OpenZeppelin's SafeCast to perform the operation.

Alternatively, we advise the usage of the VaultFactory::instanceAt function that is utilized by other similar verifiers as it contains a built-in token existence check.

Alleviation (7a4e1dc948):

The Arcade XYZ team proceeded with utilizing the VaultFactory::instanceAt function that is safe to utilize with any collateralId. However, an if check was introduced after all VaultFactory::instanceAt invocations throughout the codebase that ensures the vaultAddress when cast is equal to the collateralId.

We would like to note that utilization for the VaultFactory::instanceAt function directly is safe as it will perform an ERC721::_exists call, ensuring that the tokenId supplied is not out of bounds. As such, we advise all these if statements to be safely omitted.

Given that the original vulnerability described has been addressed, we do consider this exhibit fully alleviated despite the potential for an optimization.

Alleviation (45ccaa43fa):

The Arcade XYZ team assessed that while the additional check introduced is redundant, they wish to retain it so as to decouple the validation logic from the VaultFactory to each predicate independently. As such, we consider this exhibit fully alleviated with no follow-up action necessary.