GivingThanks

Summary

The GivingThanks::donate function is vulnerable to a reentrancy attack due to improper ordering of state changes and external calls. An attacker can exploit this vulnerability to re-enter the donate function multiple times in a single transaction, minting multiple NFTs without making additional donations. This attack remains possible even after correcting the isVerified function, as the donate function does not follow the Checks-Effects-Interactions (CEI) pattern.

Vulnerability Details

Vulnerable Code:

Issue:

• The external call to the charity address is made before updating critical state variables like tokenCounter.

• An attacker-controlled charity contract can implement a fallback function (receive()) that re-enters the donate function.

• Since state changes occur after the external call, the attacker can repeatedly re-enter donate, minting multiple NFTs.

Proof of Concept:

Note: Before running the test, ensure that the constructor in the GivingThanks contract is corrected to properly initialize the registry variable. Update the constructor on line 16 as follows:

The following test demonstrates the reentrancy attack:

Explanation:

• The ReentrantContract is registered and verified as a charity.

• When donate is called, the Ether is sent to ReentrantContract, triggering the receive() function.

• The receive() function re-enters donate, allowing the attacker to mint multiple NFTs without additional donations.

• The test confirms that 11 NFTs are minted instead of 1.

Note: This attack works even if the isVerified function is correctly implemented, as the vulnerability lies in the ordering of operations within the donate function.

Impact

• Unauthorized Minting of NFTs: Attackers can mint multiple NFTs without making corresponding donations.

• Financial Loss: Potential loss of funds if the contract holds Ether or if NFTs have monetary value.

• State Corruption: Reentrancy can lead to inconsistent state, affecting the contract's integrity.

• Reputation Damage: Exploitation undermines user trust and platform credibility.

Tools Used

• Forge (Foundry): For writing and running the test cases.

• Manual Code Analysis: To identify the improper order of operations.

• Solidity Documentation: For understanding the behavior of external calls and reentrancy.

Recommendations

• Apply the Checks-Effects-Interactions Pattern:

  Rearrange the donate function to update state variables before making external calls:

  function donate(address charity) public payable {

  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");

  }

• Use Reentrancy Guards:

  Import OpenZeppelin's ReentrancyGuard and apply the nonReentrant modifier to the donate function:

  import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

  ​

  contract GivingThanks is ERC721URIStorage, ReentrancyGuard {

  // ...

  ​

  function donate(address charity) public payable nonReentrant {

  // Function body

  }

  }

• Avoid Calling Untrusted Contracts:

  Consider whether it's necessary to send Ether directly to the charity address or if an alternative mechanism can be used.

• Implement Access Control:

  Ensure that only legitimate interactions are permitted by enforcing strict access controls and validations.

By applying these recommendations, the contract will prevent reentrancy attacks, ensuring that state changes occur before any external interactions, thus maintaining the contract's integrity and security.