salvorio / salvor-contracts

1 stars 0 forks source link

[SEE-05M] Insufficient Validation of Token Payload #15

Closed HKskn closed 3 months ago

HKskn commented 8 months ago

SEE-05M: Insufficient Validation of Token Payload

Type Severity Location
Input Sanitization SalvorExchange.sol:L232-L234

Description:

The SalvorExchange::acceptOffer function accepts a LibOrder::Token argument that is not necessarily attached to the NFT collection the input LibOrder::Offer is about.

Impact:

It is presently possible for a seller to spoof the validator into signing a LibOrder::Token payload for NFT collection A and NFT ID X while utilizing it for NFT collection B and the same NFT ID X.

Example:

/**
 * @notice Accepts an individual offer.
 * @param offer The offer to be accepted.
 * @param signature The signature corresponding to the offer.
 * @param token The token associated with the offer.
 * @param tokenSignature The signature of the token.
 */
function acceptOffer(LibOrder.Offer memory offer, bytes memory signature, LibOrder.Token memory token, bytes memory tokenSignature) internal {
    require(keccak256(abi.encodePacked(offer.salt)) == keccak256(abi.encodePacked(token.salt)), "salt does not match");
    address buyer = _validateOffer(offer, signature);
    address seller = msg.sender;
    require(buyer != seller, "signer cannot redeem own coupon");
    require(offer.bid > 0, "non existent offer");
    require((block.timestamp - offer.startedAt) < offer.duration, "offer has expired");

    bytes32 offerKeyHash = LibOrder.hashOfferKey(offer);
    require(!offerFills[offerKeyHash], "offer cancelled");
    require(offer.size > sizes[offerKeyHash], "size is filled");
    require(_hashTypedDataV4(LibOrder.hashToken(token)).recover(tokenSignature) == validator, "token signature is not valid");
    require(token.sender == msg.sender, "token signature does not belong to msg.sender");
    require(token.blockNumber + blockRange > block.number, "token signature has been expired");

    if (!offer.isCollectionOffer) {
        require(token.tokenId == offer.tokenId, "token id does not match");
    } else {
        require(keccak256(abi.encodePacked(offer.traits)) == keccak256(abi.encodePacked(token.traits)), "traits does not match");
    }

    sizes[offerKeyHash] += 1;

    IAssetManager(assetManager).payMP(buyer, seller, offer.nftContractAddress, token.tokenId, offer.bid);

    emit AcceptOffer(offer.nftContractAddress, token.tokenId, buyer, offer.salt, offer.bid);
}

Recommendation:

We advise the code to, at minimum, introduce the NFT collection address to the validated LibOrder::Token structure and to validate that it matches the input offer NFT collection address, preventing spoofed signatures from being utilized with any collection.

HKskn commented 8 months ago

added require(offer.nftContractAddress == token.nftContractAddress, "contract address does not match");