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

Missing Access Control for `GivingThanks::updateRegistry` Allows Anyone to Update the Registry Address

trashpirate

Summary

The function GivingThanks::updateRegistryas no access control and anyone can call it to update the registry address.

Vulnerability Details

Even though a owner is specified in the constructor it is not used to restrict access for the GivingThanks::updateRegistry function. This means anyone could call GivingThanks::updateRegistryand update the registry address that points to a different contract instance which makes the registry vulnerable to manipulation.

Proof of Concept

  1. Deploy GivingThanks contract with registry

  2. User deploys their own registry contract

  3. Malicious user calls GivingThanks::updateRegistry with manipulated registry address

  4. Donor can donate to unverified charity

Code:

The following test demonstrates such a scenario:

function testRegistryManipulation() public {
address maliciousUser = makeAddr("maliciousUser");
​
// user deploys their own registry contract
vm.prank(maliciousUser);
CharityRegistry maliciousRegistry = new CharityRegistry();
​
// user registrs their own charity
vm.prank(maliciousUser);
maliciousRegistry.registerCharity(maliciousUser);
​
// user verifies their own charity
vm.prank(maliciousUser);
maliciousRegistry.verifyCharity(maliciousUser);
​
// user update registry address in GivingThanks contract
vm.prank(maliciousUser);
charityContract.updateRegistry(address(maliciousRegistry));
​
// donor donates to unverified charity
vm.deal(donor, 10 ether);
vm.prank(donor);
charityContract.donate{value: 1 ether}(maliciousUser);
​
// malicious user receives the money
assertEq(maliciousUser.balance, 1 ether);
}

Impact

The verification process is vulnerable to manipulation. Someone could easily deploy a custom registry contract that contains illegitimate charities without proper verification by the admin.

Tools Used

Foundry, manual review

Recommendations

Add check that allows only the owner to update the registry address (see diff below). In addtion, a function to change the owner (access controlled) is recommended. This code could also be written as a modifier. Or as an alternative, use modifier specified in Ownable.solby the OpenZeppelin library via import and inheritance.

+ error GivingThanks__NotAuthorized();
​
+ if (msg.sender != owner){
+ revert GivingThanks__NotAuthorized();
+ }
Updates

Lead Judging Commences

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