sherlock-audit / 2024-08-flayer-judging

2 stars 0 forks source link

Muscular Admiral Marmot - Lack of Input Validation for `listing.created` Parameter in Relisting #799

Closed sherlock-admin4 closed 1 month ago

sherlock-admin4 commented 1 month ago

Muscular Admiral Marmot

Low/Info

Lack of Input Validation for listing.created Parameter in Relisting

summary

When creating or modifying a listing, the listing.created parameter is overwritten with block.timestamp. However, during relisting, listing.created remains unchanged. This lack of validation can result in unintended behaviors, including listings being stuck indefinitely or incorrect deposits.

Scenario 1: Incorrect Deposit

Alice creates a listing for an NFT, and Bob later relists this NFT, setting the listing.created parameter to a past date (block.timestamp - listing.duration). This creates a large discount on the totalPrice, potentially causing it to be reduced to zero.

    function getListingPrice(....) {
         -- SNIP -- 
            uint discount = (multiple * (block.timestamp - listing.created)) / listing.duration;
            return (true, totalPrice - discount);
    }

discount = ( 1e18 * (86400 - 0) / 86400) = 1e18

The discount calculation will be:

 discount = (1e18 * (86400 - 0) / 86400) = 1e18
 return (true, totalPrice - discount); 

This results in a totalPrice of 0 because 1e18 - 1e18:

In the fillListings function:

function fillListings(FillListingsParams calldata params) public nonReentrant lockerNotPaused {

    ownerReceives = _tload(FILL_PRICE) - (ownerIndexTokens * 1 ether * 10 ** _collectionToken.denomination());

    if (ownerReceives != 0) {
        _deposit(owner, address(_collectionToken), ownerReceives);
        totalPrice += ownerReceives;
    }
    _collectionToken.transferFrom(msg.sender, address(this), totalPrice);

}

ownerReceives = 0 because (1e18) - (1e18) _collectionToken.transferFrom(msg.sender, address(this), totalPr0ice); will deposit 0

Scenario 2: DOS

when you relist with a listing.created in the future it will DOS the listing being unable to be filled or cancled due to underflow in tax calculation until the listing.created matches the block.timestamp

  listings.relist({
            _listing: IListings.CreateListing({
                collection: address(erc721a),
                tokenIds: _tokenIdToArray(1),
                listing: IListings.Listing({
                    owner: _lister2,
                    created: uint40(25000 days),   <================== 
                    //  created: uint40(block.timestamp),
                    duration: durationVar,
                    floorMultiple: 200
                })
            }),
            _payTaxWithEscrow: false
        });
        function _resolveListingTax(Listing memory _listing, address _collection, bool _action) private returns (uint fees_, uint refund_) {

        -- SNIP --

            console2.log("_listing.duration",_listing.duration);  //604800
            console2.log("(block.timestamp",block.timestamp); //1209600
            console2.log("_listing.created)",_listing.created); //2160000000 ( in the future)

            refund_ = (_listing.duration - (block.timestamp - _listing.created)) * taxPaid / _listing.duration;

            console2.log("refund_",refund_);
        }

block.timestamp - listing.created (1209600 - 2160000000) will underflow because listing.created is in the future. The listing would be able to be canceled in filled in 25 years.

impact

Code snippet

https://github.com/sherlock-audit/2024-08-flayer/blob/main/flayer/src/contracts/Listings.sol#L665

Remediation

In the relisting function in Listings.sol overwrite the listing.created to time.blockstamp as done in creating/modifying listings.