anon339900 - The function transferOwnership does not update anything

[sherlock-admin2]

anon339900

High

The function transferOwnership does not update anything

Summary

The function transferOwnership initializes a struct called listing and changes the owner there but mistakenly does not set the _listings array to that updated struct.

Root Cause

function transferOwnership(address _collection, uint _tokenId, address payable _newOwner) public lockerNotPaused {
        // Prevent the listing from being transferred to a zero address owner
        if (_newOwner == address(0)) revert NewOwnerIsZero();

        // Ensure that the caller has permission to transfer the listing
        Listing storage listing = _listings[_collection][_tokenId];
        if (listing.owner != msg.sender) revert CallerIsNotOwner(listing.owner);

        // Update the owner of the listing
@>   listing.owner = _newOwner;
        emit ListingTransferred(_collection, _tokenId, msg.sender, _newOwner);
    }

The problem with this function is that it takes the specific listing set by the user to be transferred stores it in Listing storage listing, then updates the owner only to listing. This is problematic because all functions are comparing _listings to compare the owner of a listing and second this is not an array that can hold multiple users like _listings it is a struct that can only process one user at a time so when a user is "updated" and another user calls this function it will change the contents to the second users parameters.

There are two instances of this 1, 2

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. A user calls a function like reserve in the listings contract
2. It executes and reaches transferOwnership function
3. It does not update _listings and does technically not update the owner

Impact

The function e.g. reserve will not work correctly and will not transfer the ownership even though all the conditions shall be met (payment to owner).

PoC

No response

Mitigation

Instead of updating the storage listing update _listings or modify the code like this:

    function transferOwnership(address _collection, uint _tokenId, address payable _newOwner) public lockerNotPaused {
        // Prevent the listing from being transferred to a zero address owner
        if (_newOwner == address(0)) revert NewOwnerIsZero();

        // Ensure that the caller has permission to transfer the listing
--      Listing storage listing = _listings[_collection][_tokenId];
++      Listing memory listing = _listings[_collection][_tokenId]; // use memory to save gas
        if (listing.owner != msg.sender) revert CallerIsNotOwner(listing.owner);

        // Update the owner of the listing
        listing.owner = _newOwner;
++     _listings[_collection][_tokenId].owner =  listing.owner = _newOwner;
        emit ListingTransferred(_collection, _tokenId, msg.sender, _newOwner);
    }