6.1 Reentrancy in makeOffer function allows attackers to drain funds in Marketplace
The makeOffer function in the Marketplace.sol contract is vulnerable to a reentrancy attack because it does not use a reentrancy guard (e.g., the nonReentrant modifier). The issue arises in the following scenario:
* When a user makes an offer (via makeOffer), the contract first checks if an existing offer is active. If so, it refunds the existing offer by adding its amount to escrowBalances.
* The function then calls an external contract via IERC20(paymentToken).safeTransferFrom(msg.sender, address(this), amount).
* Since paymentToken is provided by the caller, an attacker could supply a malicious token contract that implements a reentrant fallback. When this external call is made, the malicious token could trigger a reentrant call to makeOffer before the state has been fully updated.
* By repeatedly reentering the function, the attacker can increase the escrowBalances for their address, enabling them to eventually withdraw more funds than they originally deposited and drain funds from the Marketplace.
The vulnerability specifically occurs because the state update (saving the new offer) happens after the external call, which violates the checks-effects-interactions pattern and leaves the contract exposed.
The problematic portion of the code is as follows:
function makeOffer(
uint256 tokenId,
uint256 duration,
address paymentToken,
uint256 amount
) external payable validDuration(duration) validPaymentToken(paymentToken) {
require(landPixelContract.ownerOf(tokenId) != msg.sender, "Cannot self-offer");
// Check for existing offer from this user
Offer storage existingOffer = offers[tokenId][msg.sender];
if (existingOffer.active) {
// Refund existing offer first
if (existingOffer.paymentToken == address(0)) {
escrowBalances[msg.sender][address(0)] += existingOffer.amount;
} else {
escrowBalances[msg.sender][existingOffer.paymentToken] += existingOffer.amount;
}
}
// Handle payment
if (paymentToken == address(0)) {
require(msg.value == amount, "Incorrect ETH amount");
} else {
require(msg.value == 0, "ETH not accepted");
require(amount > 0, "Amount must be > 0");
//@audit Reentrant function call.
IERC20(paymentToken).safeTransferFrom(msg.sender, address(this), amount);
}
// Create new offer
offers[tokenId][msg.sender] = Offer({
offerer: msg.sender,
amount: amount,
startTime: block.timestamp,
duration: duration,
active: true,
paymentToken: paymentToken
});
emit OfferMade(tokenId, msg.sender, amount, paymentToken);
}
Recommendation
Add the nonReentrant modifier (from OpenZeppelin’s ReentrancyGuard) to the makeOffer function to prevent reentrant calls. For example:
function makeOffer(
uint256 tokenId,
uint256 duration,
address paymentToken,
uint256 amount
) external payable nonReentrant validDuration(duration) validPaymentToken(paymentToken) {
// ... remaining logic ...
}
Resolution
Fixed in commit 6e78bb
6.2 Incorrect Computation of User Rewards
The contract tracks the cumulative rewards earned per NFT using the accumulatedRewardsPerShare variable. This variable is crucial for determining the rewards accrued by each user.
The expected behavior is that accumulatedRewardsPerShare should be updated whenever the per-block rewards change. The per-block rewards are determined by: a) The number of staked NFTs. b) The total rewards per block, which depends on remainingAllocation.
However, the contract fails to update accumulatedRewardsPerShare when remainingAllocation changes, leading to incorrect reward calculations. As a result, users may receive inaccurate rewards, potentially causing discrepancies in the reward distribution.
Recommendation
Ensure that accumulatedRewardsPerShare is updated whenever remainingAllocation or the number of staked NFTs changes to maintain accurate reward calculations.
Resolution
Fixed in commit cd21c6
6.3 buyNow(...) function does not check if the listing has expired
The buyNow(...) function is used to purchase tokens listed with either:
SellType == FixedPrice, or;SellType == Auction when buyNowPrice != 0;
Each listing has a defined lifespan, determined by its duration and the timestamp when it was created. However, the buyNow(...) function does not check whether the listing has expired.
This can lead to two unintended behaviors:
1. Fixed-Price listings: Users can still purchase a listing even after its valid duration has passed; 2. Auction listings: Users can directly buy a token using buyNowPrice, even if the auction has ended, as long as finalizeAuction(...) has not been called;
Recommendation
Consider validating the listing’s lifespan within the buyNow(...) function to ensure purchases are only allowed if the listing is still active.
Resolution
Fixed in commit 0e72ee.
6.4 buyNow function can be front-run by sellers
When an NFT marketplace listing is placed, the seller can optionally set a buy now price. For SaleType.Auction sales, the buyNowPrice parameter is used as the buy now price. For SaleType.FixedPrice sales, highestBid parameter is used as the buy now price.
function listForSale(
uint256 tokenId,
uint256 startingPrice,
SaleType saleType,
uint256 duration,
uint256 buyNowPrice,
address paymentToken
) external validDuration(duration) validPaymentToken(paymentToken) {
//... REDACTED FOR BREVITY ...
listings[tokenId] = Listing({
saleType: saleType,
seller: msg.sender,
highestBidder: msg.sender,
highestBid: startingPrice,
startTime: block.timestamp,
duration: duration,
active: true,
// Buy now price is set here
buyNowPrice: buyNowPrice,
paymentToken: paymentToken,
escrowedAmount: 0
});
By calling the buyNow function with the corresponding tokenId, buyer can purchase the NFT token immediately without waiting for auction to happen.
function buyNow(uint256 tokenId) external payable nonReentrant {
Listing storage listing = listings[tokenId];
require(msg.sender != listing.seller, "Cannot buy own listing");
require(listing.active, "Not for sale");
//... REDACTED FOR BREVITY ...
}
Notice that only tokenId is taken as a function argument in the buyNow function. Since the buyers only specify the tokenId they are purchasing, this can lead to front-running attacks where the seller can change the buy now price before the purchase is made. This is possible because sellers can unlist the NFT and re-list again with different buy now prices. This is only possible for SaleType.FixedPrice type sales as only those types of listing are allowed to be unlisted. Here’s a sample attack scenario:
1. Seller creates a market listing using the listForSale function setting sale type -> SaleType.FixedPrice; 2. Buyer notices the listing and calls buyNow function with the tokenId in the first step; 3. Seller front-runs the buyNow transaction and first calls the unlist function and then calls listForSale function with different buy now prices; 4. Buyer’s buyNow transaction goes through and purchase is made for a much higher price than buyer intended;
Recommendation
Each listing created via the listForSale function should have a unique id. After a listing is created, they should be immutable. Users should use the unique listing ids to interact with the protocol.
Resolution
Fixed in commit a50086.
6.5 acceptOffer function can be front-run
In the Marketplace contract, Landpixel NFT owners can auction their NFTs or accept offers made to their NFTs. If an offer is made to a Landpixel NFT, the NFT owner will call the acceptOffer(uint256 tokenId, address offerer) function to accept the offer.
function acceptOffer(uint256 tokenId, address offerer) external nonReentrant {
require(landPixelContract.ownerOf(tokenId) == msg.sender, "Not the token owner");
Offer storage offer = offers[tokenId][offerer];
require(offer.active, "Offer inactive");
require(block.timestamp < offer.startTime + offer.duration, "Offer expired");
uint256 amount = offer.amount;
address paymentToken = offer.paymentToken;
// Deactivate the accepted offer
offer.active = false;
// Calculate fees
uint256 feeAmount = (amount * marketplaceFee) / 10000;
uint256 sellerAmount = amount - feeAmount;
// Transfer NFT
landPixelContract.transferFrom(msg.sender, offerer, tokenId);
// Transfer payment
if (paymentToken == address(0)) {
(bool success, ) = payable(msg.sender).call{value: sellerAmount}("");
require(success, "Seller payment failed");
(bool feeSuccess, ) = payable(landBank).call{value: feeAmount}("");
require(feeSuccess, "Fee transfer failed");
} else {
IERC20(paymentToken).safeTransfer(msg.sender, sellerAmount);
IERC20(paymentToken).safeTransfer(landBank, feeAmount);
}
emit SaleFinalized(tokenId, offerer, amount, paymentToken);
}
However, the contract doesn’t account for the fact that offers can be changed after they’re made. This means a malicious buyer could change their offer right before the NFT owner accepts it, potentially lowering the offer after the owner has already agreed to the original terms. Imagine the following attack scenario:
1. Attacker makes an offer to a Landpixel NFT via makeOffer(...) function; 2. NFT owner accepts the offer by calling acceptOffer(...) and publishes tx onchain; 3. Attacker front-runs the NFT owner’s transaction and updates the offer with makeOffer(...) function; 4. NFT owner’s acceptOffer(...) is executed and owner sells their NFT for less than agreed amount;
Recommendation
Track offers by storing unique identifiers. Each offer should have a unique identifier so the NFT owner can accept without taking a risk.
Resolution
Fixed in commit 49bdd5.
6.6 Users can bid with the same amount if highestBid is too low
In the Marketplace contract, users can place bids on listings with SaleType == Auction. The contract enforces that each new bid must exceed the previous highestBid by a percentage of it, using the following calculation:
...
uint256 minBid = listing.highestBid + ((listing.highestBid * MIN_BID_INCREMENT_BPS) / 10000);
require(bidAmount >= minBid, "Bid too low");
...
However, if listing.highestBid * MIN_BID_INCREMENT_BPS results in a value lower than 10000, the division by 10000 will round down to zero (in integer division). As a result, minBid will be equal to highestBid, allowing users to place bids at the same price.
This behavior can lead to auction sniping, where a bidder submits a last-minute bid at the same price as the current highest bid, potentially undermining the intended competitive bidding process.
Recommendation
Consider modifying the bid enforcement logic to ensure minBid is always strictly greater than highestBid. Possible solutions include:
- Setting a minimum price to prevent
minBid from equaling highestBid; - Explicitly enforcing
bidAmount > highestBid in the require statement;
Resolution
Fixed in commit 07ee7c.
Update from Nethermind Security
This wrongly enforces the bidAmount to be strictly bigger than minBid always. bidAmount should be strictly higher than previous bid, but greater or equal to minBid
Update from the client
Fixed in commit 91bb48.
6.7 buyNow(...) function does not use the correct buy now price
In the buyNow(...) function, buy now price is different depending on the SaleType:
uint256 price = listing.saleType == SaleType.FixedPrice ? listing.highestBid : listing.buyNowPrice;
- For the
SaleType.FixedPrice, highestBid is used as the price; - For the
SaleType.Auction, buyNowPrice is used as the price;
This can lead to ambiguity for the users and they might assume the incorrect buy now price is valid. A seller can put a very high highestBid price and a very low buyNowPrice for a fixed price sale. The users might assume the buyNowPrice is what they’ll pay. However, they’ll pay the highestBid price and will pay more than they intended.
Recommendation
Only use the buyNowPrice in the buyNow function.
Resolution
Fixed in commit e13dc9.
6.8 mintLandPixels cannot be executed after reaching reward allocation
The mintLandPixels(...) function allows users to mint new NFTs by paying the pixelCost per NFT. In return, users receive both the NFT and DSTRX rewards.
However, if minting a new NFT would cause the total user rewards allocation to be exceeded, the function reverts, preventing any further minting. This makes it impossible to mint new NFTs even if users are willing to forgo rewards.
Recommendation
Modify the function logic to allow minting even when the user rewards allocation is exhausted. Instead of reverting, conditionally distribute rewards based on the remaining allocation.
Resolution
Fixed in commit 9c6eff.
6.9 Marketplace receives zero fees if paid amount is too low
The Marketplace contract gets a fee from every sell done through it and send it to the LandBank contract.
// Calculate fees
// @audit - Fee computation rounds down
uint256 feeAmount = (amount * marketplaceFee) / 10000;
uint256 sellerAmount = amount - feeAmount;
If the price of the sell is to low, the computation of the fees will round towards zero making the fee amount zero.
Recommendation
Consider rounding up if fees must be enforced.
Resolution
Fixed in commit 0cbb98.
