Description

The _data calldata parameter passed to buyNow(...) requires a consistency check against the _tokenId parameter.

The _tokenId passed is recorded in

LibAppStorage.ds().nftIndex[param.collection][
    param.tokenId
] = composeNFTInfo(param);

where nftInfo.isCollateral is set to true by composeNFTInfo(...).

However, the actual NFT ordered from the market is specified in _data, which is not validated to match the given _tokenId. This allows an attacker to purchase a mismatching NFT, which is sent to the vault. The NFT corresponding to the _tokenId argument is marked as collateral for the loan instead of the one that was actually received. Therefore, the token can be withdrawn from the vault, as the following check in VaultFacet::withdrawNFT(...) will not revert the transaction:

if (LibAppStorage.ds().nftIndex[_collection][_tokenId].isCollateral) {
    revert InvalidWithdrawal();
}

Impact

This vector makes the current implementation vulnerable to several attacks. For example, buyNow can be called with tokenId = 10 and calldata _data containing tokenId = 15. The order will process and an NFT with tokenId = 15 will be purchased. The NFT can then be withdrawn while having only paid the down payment.

Recommendations

Validation checks should be added to ensure that the tokenId and collection passed in are consistent with the tokenId and collection passed in calldata _data. Additionally, validation modules should be added to the LooksRareAdapter and SeaportAdapter to validate all other order parameters. Currently, only the order selectors are validated. This additional lack of checks opens up the possibility for more missing validation exploits on other variables. However, the core of the vulnerability is the same, and so we have grouped them all into one finding.

Remediation

Voyage has incoporated the necessary validation checks for tokenId and collection in commit 7937b13a↗. They have also included additional validation checks for isOrderAsk and taker in LooksRareAdapter and fulfillerConduitKey in SeaportAdapter in commit 7937b13a↗. These are critical validations checks to have included and we applaud Voyage for their efforts. However, there may still be other parameters that require validation checks in the orders and we suggest Voyage perform a comprehensive review of all of the parameters in determine if there are any outstanding validation checks that may be necessary.