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

Misleading return from `CharityRegistry::isVerified()` allows users to donate to unverified charities

somasine

Relevant Links

Summary

The function CharityRegistry::isVerified is intended to check if a charity address has been verified by the CharityRegistry admin. However, a vulnerability exists in this function's logic where it instead checks for resgistered charities. The misleading function is then used in GivingThanks::donate, which should prevent donors from donating to an unverified charity. Because of this vulnerability, users can register themselves, or a malicious address, and potentially mislead donors into thinking that a charity is verified, when in reality it is not, having that donor donate funds to that malicious address.

Vulnerability Details

The contract CharityRegistry has two mappings which handle which charity address has been registered, and which charity address has been verified:

mapping(address => bool) public verifiedCharities;
mapping(address => bool) public registeredCharities;

Furthermore, any charity/user can register a charity, so that the admin of the CharityRegistry contract can call on verifyCharity to verify the given charity.
To check if a charity has been verified, the CharityRegistry contract has provided a function isVerified. However a serious vulnerability lies within this function due to a check on the wrong mapping. This function should be using the verifiedCharities but instead uses registeredCharities, which is highlighted below:

function isVerified(address charity) public view returns (bool) {
@> return registeredCharities[charity];
}

If a user/charity register themselves and the admin does NOT verify them, the isVerified function will return true, despite verifiedCharities[charity] returning false.

This vulnerability becomes more problematic because of its use in the GivingThanks::donate function as seen below:

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

The donate function SHOULD prevent users from being able to donate to unverified charities, but because CharityRegistry::isVerified checks the mapping of registeredCharities and not verifiedCharities, donors can donate to any "charity" address, so long as they or another user registers that address.

Impact

Likelihood: High/Medium
Severity: High/Medium
Because the isVerified function checks the registeredCharities and that any user can register themselves or another address, funds from unsuspecting donors could be sent to potentially malicious actors, believing those actors to be verified.

POC

Please add the following test under test/GivingThanks.t.sol:

modifier fixCharity() { // To fix the constructor's issue :/
vm.prank(admin);
charityContract.updateRegistry(address(registryContract));
_;
}
​
function test_UserCanDonateToUnverifiedCharity() public fixCharity {
address maliciousUser = makeAddr("maliciousUser");
vm.prank(maliciousUser); // malicious user registers themselves
registryContract.registerCharity(maliciousUser);
vm.stopPrank();
​
// Show that the new Charity is registered
assertEq(registryContract.registeredCharities(maliciousUser), true);
// Show that the new Charity is NOT verified
assertEq(registryContract.verifiedCharities(maliciousUser), false);
// Show that despite NOT being verified, the `isVerified` function returns true
assertEq(registryContract.isVerified(maliciousUser), true);
​
vm.deal(donor, 1 ether);
// Charitable user donating to what they think is a verified charity
vm.prank(donor);
charityContract.donate{value: 1 ether}(maliciousUser);
vm.stopPrank();
}

And then run the following command:

forge test --mt test_UserCanDonateToUnverifiedCharity

Tools Used

  • Manual Review

Recommendations

In order to fix this issue, please change the CharityRegistry::isVerified function to the following:

function isVerified(address charity) public view returns (bool) {
+ return verifiedCharities[charity];
- return registeredCharities[charity];
}
Updates

Lead Judging Commences

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

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.