First Flight #9: Soulmate
First Flight #9
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Missing ERC721 receiver check in `Soulmate::mintSoulmateToken` function

bube

Summary

The Soulmate::mintSoulmateToken function mints an ERC721 token without the necessary checks to ensure that the receiver of a minted token can handle ERC721 tokens, which could lead to tokens being locked in the contract that do not support the ERC721 token interface.

Vulnerability Details

The Soulmate contract is an ERC721 token designed to represent soulmate pairings. The Soulmate::mintSoulmateToken function mints a new token when two soulmates are reunited. The contract is intended to be soulbound, meaning that tokens cannot be transferred after minting. For a token minting the function uses ERC721::_mint function.

​
function mintSoulmateToken() public returns (uint256) {
// Check if people already have a soulmate, which means already have a token
address soulmate = soulmateOf[msg.sender];
if (soulmate != address(0))
revert Soulmate__alreadyHaveASoulmate(soulmate);
​
address soulmate1 = idToOwners[nextID][0];
address soulmate2 = idToOwners[nextID][1];
if (soulmate1 == address(0)) {
idToOwners[nextID][0] = msg.sender;
ownerToId[msg.sender] = nextID;
emit SoulmateIsWaiting(msg.sender);
} else if (soulmate2 == address(0)) {
idToOwners[nextID][1] = msg.sender;
// Once 2 soulmates are reunited, the token is minted
ownerToId[msg.sender] = nextID;
soulmateOf[msg.sender] = soulmate1;
soulmateOf[soulmate1] = msg.sender;
idToCreationTimestamp[nextID] = block.timestamp;
​
emit SoulmateAreReunited(soulmate1, soulmate2, nextID);
​
@> _mint(msg.sender, nextID++);
}
​
return ownerToId[msg.sender];
}
​

The implementation of the _mint function is the following:

​
function _mint(address to, uint256 id) internal virtual {
require(to != address(0), "INVALID_RECIPIENT");
​
require(_ownerOf[id] == address(0), "ALREADY_MINTED");
​
// Counter overflow is incredibly unrealistic.
unchecked {
_balanceOf[to]++;
}
​
_ownerOf[id] = to;
​
emit Transfer(address(0), to, id);
}
​

We can see that the _mint function doesn't perform a check if the account to mint the NFT to is a smart contract that can handle ERC721 tokens. So tokens can be minted to non-ERC721 receivers.

Impact

The ERC721 standard includes an optional extension for safe transfers, which requires that the receiving contract implements the onERC721Received function. This function must return a value to indicate that the contract can handle ERC721 tokens. If the receiving contract does not return this value, the transfer should be reverted.

The ERC721::_mint function does not perform this check, which means that if a token is minted to a contract address that does not implement the onERC721Received function, the token could become permanently locked within that contract.
The absence of the ERC721 receiver check poses a high risk of token loss if a token is minted to a contract that cannot handle ERC721 tokens.

The test function test_failIfCallerCanNotReceiveERC721Token shows that the Soulmate::mintSoulmateToken function doesn't revert when the contract address badReceiver (the address can not receive ERC721 token) is calling the Soulmate::mintSoulmateToken.

​
function test_failIfCallerCanNotReceiveERC721Token() public {
vm.prank(soulmate1);
soulmateContract.mintSoulmateToken();
​
//the caller is a contract that can not accept ERC721 token
vm.prank(address(badReceiver));
vm.expectRevert();
soulmateContract.mintSoulmateToken();
}

Tools Used

Manual Review

Recommendations

Use ERC721::_safeMint function instead of ERC721::_mint function. But add a nonReentrant modifier and follow the checks-effects-interactions pattern in Soulmate::mintSoulmateToken function, because the onERC721Received callback in the _safeMint function introduces a reentrancy vector.

Updates

Lead Judging Commences

0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-safemint

Low severity, - If a user utilizes an EOA, the check is not required. - If a user utilizes a contract that they own to mint soulmate tokens, than check is required. However, this would rely on user error minting. Since there is no mention that ownership of token must be from EOAs, I believe low severity is appropriate.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.