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

Registry contract not set properly , Making the whole system broken !

mo_

Summary

The GivingThanks contract incorrectly sets the registry variable to the caller's address instead of the provided _registry address during the contract's construction. This makes the whole givintThankscontract unusable .

Vulnerability Details

In the constructor of the GivingThanks contract, the registry variable is assigned the value of msg.sender instead of the _registry parameter passed to the constructor. This means that the contract will not interact with the intended CharityRegistry contract , registry will be set to the caller .

Code PoC

Add this file to GivingThanks.t.sol :

function testRegistryHasNoCode() public {
address registryAddress = address(charityContract.registry());
uint256 codeSize;
assembly {
codeSize := extcodesize(registryAddress)
}
assertEq(codeSize, 0, "Registry address should have no code");
}

run

forge test --match-test "testRegistryHasNoCode"

Output :

Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[PASS] testRegistryHasNoCode() (gas: 13722)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.97ms (2.03ms CPU time)

Impact

This vulnerability can lead to several issues:

  • The contract will not verify charities correctly, as it is not referencing the intended CharityRegistry. Calls will revert and Donations cannot be made .

Tools Used

  • Manual code review

    Cursor

Recommendations

  • Update the constructor to correctly assign the registry variable using the _registry parameter:

constructor(address _registry) ERC721("DonationReceipt", "DRC") {
registry = CharityRegistry(_registry); // Correct assignment
owner = msg.sender;
tokenCounter = 0;
}

  • Implement access control mechanisms to ensure that only authorized addresses can update the registry in the future.

Updates

Lead Judging Commences

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

finding-bad-registry-set-at-construction

Likelyhood: High, the parameter is not well used and won't be set. Impact: Low, can be changed with the setter and no one will be able to donate to malicious charity.

Support

FAQs

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