Relevant Links
Summary
The GivingThanks contract uses an unsafe NFT mint method _mint() instead of the safer counterpart _safeMint(). As a result, tokens could be permanently locked into a contract that doesn't support the given ERC721 tokens.
Vulnerability Details
The GivingThanks contract implements an NFT minting within the donate function below
function donate(address charity) public payable {
require(registry.isVerified(charity), "Charity not verified");
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
@> _mint(msg.sender, tokenCounter);
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
tokenCounter += 1;
}
This vulnerability stems from the _mint function which doesn't verify if the recipient can handle an associated ERC721 token. According to the ERC721 standard, a checkOnERC721Recieved call is performed within _safeMint and will pass given the following:
* @dev Performs an acceptance check for the provided `operator` by calling {IERC721-onERC721Received}
* on the `to` address. The `operator` is generally the address that initiated the token transfer (i.e. `msg.sender`).
*
* The acceptance call is not executed and treated as a no-op if the target address doesn't contain code (i.e. an EOA).
* Otherwise, the recipient must implement {IERC721Receiver-onERC721Received} and return the acceptance magic value to accept
* the transfer.
*/
function checkOnERC721Received(
address operator,
address from,
address to,
uint256 tokenId,
bytes memory data
) internal {
...
}
In essence, a contract doesn't support ERC721 if it doesn't implement the onERC721Received function, causing tokens to be permanently locked in that given contract.
Impact
Likelihood: Medium/Low
This is because I'm assuming mostly EOAs would interact with this contract.
Severity: Medium/Low
If a contract that doesn't support ERC721s is used, the NFT is just a receipt of the donation, so the value of that NFT is mainly for information.
POC
To test a contract that doesn't support ERC721s, we need to make a comparision between an ERC721 that uses _safeMint, and theGivingThanks::donate function which doesn't.
As such, I made a MockERC721.sol under the test/ directory:
import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
contract MockERC721 is ERC721 {
uint256 private _tokenId;
constructor() ERC721("Mock721", "MOCK") {}
function safeMint() external {
_safeMint(msg.sender, _tokenId);
_tokenId += 1;
}
}
Then I added the vulnerable contract at the top of the GivingThanks.t.sol file:
import {MockERC721} from "test/MockERC721.sol";
contract VulnerableContract {
function makeDonation(address _contract, address charity) external payable {
GivingThanks charityContract = GivingThanks(_contract);
charityContract.donate{value: msg.value}(charity);
}
}
Finally, I added the following test (and modifier to fix the broken charityRegistry address) inside the GivingThanksTest contract:
modifier fixCharity() {
vm.prank(admin);
charityContract.updateRegistry(address(registryContract));
_;
}
function test_UnsafeMintOnVulnerableContract() public fixCharity {
MockERC721 mockERC721 = new MockERC721();
VulnerableContract vulnerable = new VulnerableContract();
vm.deal(address(vulnerable), 10 ether);
vm.prank(address(vulnerable));
vm.expectRevert();
mockERC721.safeMint();
vulnerable.makeDonation{value: 1 ether}(address(charityContract), charity);
assertEq(charityContract.balanceOf(address(vulnerable)), 1);
}
Tools Used
Recommendations
The best way to fix this vulnerability is to replace the unsafe _mint() function with the _safeMint function:
function donate(address charity) public payable {
require(registry.isVerified(charity), "Charity not verified");
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
- _mint(msg.sender, tokenCounter);
+ _safeMint(msg.sender, tokenCounter);
// Create metadata for the tokenURI
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
tokenCounter += 1;
}
