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

Incorrect access control in `GivingThanks::updateRegistry` allows any user to update the `CharityRegistry registry`

somasine

Relevant Links

Summary

GivingThanks::updateRegistry is intended to update the charity information stored in the contract. It can be assumed that only the owner should be able to call on it since the CharityRegistry public registry; state variable is a critical component of the contracts functionality. However, due to a lack of user validation on who can successfully execute this function, any user can deploy their own CharityRegistry contract (becoming the admin of their CharityRegistry), verify themselves or their own malicious charity(s), and then update the registry info in the GivingThanks contract with that malicious information.

Vulnerability Details

The contract GivingThanks stores a key state variable CharityRegistry public registry; which is used in GivingThanks::donate to check which charity address has been verified by the admin of the CharityRegistry contract:

function donate(address charity) public payable {
@> require(registry.isVerified(charity), "Charity not verified");
(bool sent,) = charity.call{value: msg.value}("");

This variable handles a critical part of the GivingThanks platform as it ensures that donors can't donate to charities that haven't been verified first. However, the vulnerability that can break this lies in the function GivingThanks::updateRegistry provided below:

function updateRegistry(address _registry) public {
registry = CharityRegistry(_registry);
}

An attacker can easily exploit this function due to the lack of access control, and could update the registry state variable with a list of malicious charities that the attacker has validated themselves. They could do this through the following steps:

  1. Deploy their own CharityRegistry contract, making the attacker an admin

  2. Register and Validate any number of malicious charity addresses

  3. Call on GivingThanks::updateRegistry with this new malicious charity registry

  4. Unknowing donor who is convinced to donate, calls on the GivingThanks::donate, sending money to a malicious "charity"

Impact

Likelihood: High/Medium
Severity: High
The registry state variables is a critical component of the GivingThanks behavior and security. Because of the lack of access control, two scenarios could happen:

  1. Users who want to donate to a previously registered charity will be unable to, resulting in a denial of funds

  2. Users who trust the contract see a verified charity and send funds, causing those funds to be sent to an address that, in actuality, is not a charity and is a malicious actor.
    This directly affects any interaction with the contract.

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_AnyUserCanUpdateCharityRegistry() public fixCharity {
address maliciousUser = makeAddr("maliciousUser");
vm.deal(donor, 2 ether);
​
vm.prank(donor); // Donor donates 1 ether to good charity
charityContract.donate{value: 1 ether}(charity);
assertEq(charity.balance, 1 ether); // Charity receives donation
​
// Malicious user makes a new CharityRegistry contract
vm.prank(maliciousUser);
CharityRegistry badRegistryContract = new CharityRegistry();
​
// Malicious user registers and verified themselves as a charity
vm.prank(maliciousUser);
badRegistryContract.registerCharity(maliciousUser);
vm.prank(maliciousUser);
badRegistryContract.verifyCharity(maliciousUser);
​
// Malicious user then updates the registry in the charityContract
vm.prank(maliciousUser);
charityContract.updateRegistry(address(badRegistryContract));
// GivingThanks contract's registry is now the malicious charity registry
assertEq(address(charityContract.registry()), address(badRegistryContract));
​
vm.prank(donor); //Unknowing user attempts to donate again
vm.expectRevert(); // Prove that user's transaction doesn't go through :(
charityContract.donate{value: 1 ether}(charity);
}

Then run the following CLI command:

forge test --mt test_AnyUserCanUpdateCharityRegistry

Tools Used

  • Manual Review

Recommendations

To properly handle access control, I recommend adding a require statement to the top of GivingThanks::updateRegistry:

function updateRegistry(address _registry) public {
+ require(msg.sender == owner, "Unauthorized Account");
registry = CharityRegistry(_registry);
}

Once implemented, only the owner of the contract will be able to update the registry information.

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

Support

FAQs

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