salvorio / salvor-contracts

0 stars 0 forks source link

[SEE-02M] Inexplicable Deposit of Native Funds #8

Closed HKskn closed 2 months ago

HKskn commented 7 months ago

SEE-02M: Inexplicable Deposit of Native Funds

Type Severity Location
Logical Fault SalvorExchange.sol:L127-L142

Description:

The SalvorExchange::acceptOfferBatchETH function is inexplicable in the sense that it permits the msg.sender, who is treated as the seller and thus the owner of the NFT IDs, to deposit native funds to the AssetManager which ultimately will not be utilized by the transaction.

Impact:

The functionality implemented by SalvorExchange::acceptOfferBatchETH does not appear to serve a purpose and can lead to a misallocation of funds that would then need to be extracted in a secondary transaction.

Example:

/**
 * @notice Accepts a batch of offers for tokens with Ether payment in a single transaction.
 * @param offers Array of offers to be accepted.
 * @param signatures Array of signatures corresponding to each offer.
 * @param tokens Array of tokens for which the offers are made.
 * @param tokenSignatures Array of signatures corresponding to each token.
 */
function acceptOfferBatchETH(LibOrder.Offer[] calldata offers, bytes[] calldata signatures, LibOrder.Token[] calldata tokens, bytes[] calldata tokenSignatures) external payable whenNotPaused nonReentrant {
    uint256 len = offers.length;
    require(len <= 20, "exceeded the limits");
    IAssetManager(assetManager).deposit{ value: msg.value }(msg.sender);
    uint64 i;
    for (; i < len; ++i) {
        acceptOffer(offers[i], signatures[i], tokens[i], tokenSignatures[i]);
    }
}

Recommendation:

We advise the function to be omitted from the codebase as it presently does not serve any purpose and may be perceived as misleading.

HKskn commented 7 months ago

Done