Project

Summary

The smart contract contains a critical reentrancy vulnerability in the donate() function, located on line 35 of the provided contract. The vulnerability arises from the fact that the contract makes an external call to a potentially untrusted contract (charity address) before updating its internal state, particularly the tokenCounter. This exposes the contract to a reentrancy attack, where the charity contract could recursively call back into the donate() function before the token minting is complete, potentially leading to unintended behavior such as draining funds or minting tokens multiple times.

Vulnerability Details

The reentrancy vulnerability is present in the following function:

Vulnerability Explanation

In this contract, the donate() function is used for users to donate Ether to a verified charity and receive an ERC721 token in return, which serves as a receipt for their donation. Here's a breakdown of what happens:

1. Charity Validation: The contract checks if the charity is verified using registry.isVerified(charity).

2. External Call to Charity: The contract sends the donated Ether to the charity using charity.call{value: msg.value}(""). This external call is performed before the state is updated, i.e., before the tokenCounter is incremented and the token is minted.

3. Minting Token and Setting URI: After sending the Ether, the contract mints the token for the sender using _mint(msg.sender, tokenCounter) and sets the token's URI using _setTokenURI(tokenCounter, uri).

4. State Update: Finally, the tokenCounter is incremented by 1.

The problem here is that state changes (like _mint(), _setTokenURI(), and tokenCounter += 1) should ideally occur before making external calls to untrusted addresses to prevent reentrancy attacks. By making an external call (charity.call{value: msg.value}("")) before updating the contract's state, an attacker-controlled charity contract could call back into the donate() function, re-entering the contract and potentially causing unexpected behavior like:

• Reentrancy Attack: An attacker could repeatedly trigger the donate() function, minting multiple tokens or draining more Ether from the contract than intended.

This type of vulnerability is considered high-risk and could lead to significant financial losses or unauthorized minting of tokens.

Impact

• Financial Loss: The primary risk is the potential to lose Ether if the contract is drained by a reentrancy attack. An attacker could recursively call the donate() function, withdrawing more Ether than intended.

• Token Duplication: An attacker could exploit this vulnerability to mint multiple tokens, leading to a situation where they hold more tokens than they should.

• Unintended Contract Behavior: Since the minting of tokens occurs after the external call, the internal state of the contract (e.g., tokenCounter) could be manipulated by the attacker, creating invalid states or duplicate tokens.

In the worst case, the attacker could control the charity address (or use a malicious contract) to perpetuate the attack, potentially locking up funds or draining them from the contract.

Tools Used

• Manual Code Review: An in-depth analysis of the code's logic and flow, particularly the ordering of state changes and external calls.

• Foundry: Used to simulate different attack scenarios, such as a malicious charity contract that exploits the vulnerability by reentering the donate function.

• MythX: A static analysis tool to identify known security issues such as reentrancy vulnerabilities.

Recommendations

To mitigate the reentrancy risk, the contract should follow the checks-effects-interactions pattern, which dictates that state changes (like minting tokens and incrementing counters) should always occur before making external calls. Here’s the recommended code fix:

Fixed Code:

Key Changes:

1. State Changes Before External Calls: The _mint(), _setTokenURI(), and tokenCounter += 1 are now executed before sending Ether to the charity.

2. External Call After State Update: The external call to the charity contract (charity.call{value: msg.value}("")) is now made after the state changes have been applied, preventing any malicious charity contract from reentering and manipulating the state.

This pattern ensures that the contract’s state is always in a consistent and expected condition before interacting with external contracts, thereby mitigating the reentrancy risk.

Proof of Concept for Reentrancy Vulnerability

Overview:

The vulnerability arises because the contract makes an external call (charity.call{value: msg.value}("")) before changing the state (minting tokens and updating tokenCounter), which can lead to reentrancy attacks where the malicious charity contract can call back into the donate() function.

Actors:

• Attacker: An attacker who controls or can deploy a malicious charity contract. The attacker will exploit the reentrancy vulnerability by recursively calling the donate() function.

• Victim: A user donating Ether to a verified charity. The user expects to receive an NFT token as a receipt for their donation.

• Protocol: The GivingThanks contract that facilitates donations to verified charities and issues NFT receipts.

Working Test Case (Example Using Foundry):

In this PoC, the MaliciousCharity contract reenters the donate() function by calling it recursively through its receive() function. This allows the attacker to mint multiple tokens and drain Ether from the contract.

Testing with Foundry:

Use the above command in Foundry to test the vulnerability. The malicious contract (MaliciousCharity) will attempt to donate repeatedly, causing unexpected minting and draining of funds.

Conclusion:

The reentrancy vulnerability in the donate() function is a severe security risk because it allows an attacker to mint tokens multiple times or drain funds. Following the checks-effects-interactions pattern as described above is a simple but effective way to mitigate this vulnerability and ensure that the contract remains secure.