GivingThanks

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

Bug Report: Reentrancy Vulnerability in donate Function of GivingThanks.sol

Summary:
The donate function in the GivingThanks.sol contract is vulnerable to a reentrancy attack due to the external call to charity.call{value: msg.value}(""). This call is made before the state variables are updated, enabling a malicious charity contract to re-enter the function and perform additional malicious actions before the state is fully updated.

Vulnerability Details:
The vulnerability arises because the contract first sends Ether to the charity address (which could be a contract) and then performs state-changing actions such as minting a token and updating the metadata. If the charity contract is malicious and contains a fallback function, it could re-enter the donate function before the state changes, allowing an attacker to exploit the situation (e.g., by calling the donate function repeatedly and draining Ether).

Here is the vulnerable part of the code:

function donate(address charity) public payable {
require(registry.isVerified(charity), "Charity not verified");
// External call to charity before state changes
(bool sent, ) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
// State changes occur after the external call
_mint(msg.sender, tokenCounter);
string memory uri = _createTokenURI(
msg.sender,
block.timestamp,
msg.value
);
_setTokenURI(tokenCounter, uri);
tokenCounter += 1;
}

Impact:
High Impact:The ability for an external contract to re-enter the donate function can result in a reentrancy attack. This could allow a malicious contract to repeatedly call donate, draining the contract's funds and potentially minting multiple tokens for the same donation. This poses a serious risk to the integrity and security of the contract and its funds.

Tools Used:

  • Manual code review

  • Slither (for vulnerability detection)

INFO:Detectors:
Reentrancy in GivingThanks.donate(address) (src/GivingThanks.sol#21-37):
External calls:
- (sent) = charity.call{value: msg.value}() (src/GivingThanks.sol#23)
State variables written after the call(s):
- _mint(msg.sender,tokenCounter) (src/GivingThanks.sol#26)
- _balances[from] -= 1 (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#256)
- _balances[to] += 1 (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#262)
- _mint(msg.sender,tokenCounter) (src/GivingThanks.sol#26)
- _owners[tokenId] = to (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#266)
- _mint(msg.sender,tokenCounter) (src/GivingThanks.sol#26)
- _tokenApprovals[tokenId] = to (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#424)
- _setTokenURI(tokenCounter,uri) (src/GivingThanks.sol#34)
- _tokenURIs[tokenId] = _tokenURI (lib/openzeppelin-contracts/contracts/token/ERC721/extensions/ERC721URIStorage.sol#58)
- tokenCounter += 1 (src/GivingThanks.sol#36)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2
INFO:Detectors:
Reentrancy in GivingThanks.donate(address) (src/GivingThanks.sol#21-37):
External calls:
- (sent) = charity.call{value: msg.value}() (src/GivingThanks.sol#23)
Event emitted after the call(s):
- Approval(owner,to,tokenId) (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#420)
- _mint(msg.sender,tokenCounter) (src/GivingThanks.sol#26)
- MetadataUpdate(tokenId) (lib/openzeppelin-contracts/contracts/token/ERC721/extensions/ERC721URIStorage.sol#59)
- _setTokenURI(tokenCounter,uri) (src/GivingThanks.sol#34)
- Transfer(from,to,tokenId) (lib/openzeppelin-contracts/contracts/token/ERC721/ERC721.sol#268)
- _mint(msg.sender,tokenCounter) (src/GivingThanks.sol#26)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-3

Recommendations:
To mitigate the reentrancy vulnerability, we recommend the following solutions:

  1. Use the Checks-Effects-Interactions Pattern: Move all state changes (such as minting the token, setting the URI, and updating tokenCounter) before the external call to ensure that the contract state is updated before the external contract can interfere.

  2. Use a ReentrancyGuard Modifier: Implement the nonReentrant modifier from OpenZeppelin’s ReentrancyGuardto prevent multiple calls to the donate function during execution.

Solution: Updated Code:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
contract GivingThanks is ERC721URIStorage, ReentrancyGuard {
// ... rest of the code remains unchanged
function donate(address charity) public payable nonReentrant {
require(registry.isVerified(charity), "Charity not verified");
// First, perform all internal state changes
_mint(msg.sender, tokenCounter);
string memory uri = _createTokenURI(
msg.sender,
block.timestamp,
msg.value
);
_setTokenURI(tokenCounter, uri);
tokenCounter += 1;
// After state changes, execute external call
(bool sent, ) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
}
}


Explanation of the Fixes

  1. ReentrancyGuard: The nonReentrant modifier prevents reentrancy by blocking any re-entry into the donatefunction. This ensures that once the function is entered, no other calls can interrupt it until it completes.

  2. Checks-Effects-Interactions Pattern: All internal state changes (such as minting the token, setting the token URI, and incrementing the tokenCounter) are now done before the external call to charity. This prevents any reentrancy issues, as the contract’s state is fully updated before the external call is made.

Conclusion

By applying both the ReentrancyGuard and the Checks-Effects-Interactions pattern, the reentrancy vulnerability in the donate function is mitigated. These changes secure the contract by ensuring that external calls cannot interfere with the contract’s state, protecting it from malicious reentrancy attacks.

Updates

Lead Judging Commences

n0kto Lead Judge 7 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.