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

Broken access control in GivingThanks::updateRegistry function.

blacksquirrel

Summary

Any user can potentially exploit the updateRegistry function to modify the address of the registry contract.

Vulnerability Details

Proof of code:

Add this code to tests , but as there is a bug in the constructor of the GivingThankscontract , fix it before running tests .

constructor(address _registry) ERC721("DonationReceipt", "DRC") {
+ registry = CharityRegistry(_registry);
- registry = CharityRegistry(_msg.sender);
owner = msg.sender;
tokenCounter = 0;
}
function testBrokenAccess() public {
vm.prank(donor);
// change registry address to donor address
charityContract.updateRegistry(donor);
// check if donor address is equal to updated registry address
assertEq(donor, address(charityContract.registry()));
}
​

Impact

A malicious actor could potentially compromise the functionality of the GivingThanks contract by modifying the registry contract address to an invalid value.

Tools Used

Manual code review

Recommendations

Properly implement the Ownable library and add the onlyOwner modifier to the updateRegistry function.

contract GivingThanks is ERC721URIStorage, Ownable(msg.sender) {
​
+ function updateRegistry(address _registry) public onlyOwner {
// @audit-issue broken acess control
registry = CharityRegistry(_registry);
}
Updates

Lead Judging Commences

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