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

Incorrect Initialization of `registry` in `GivingThanks` Contract Constructor

spidy7301

Summary

In the GivingThanks contract, the registry variable is incorrectly initialized to msg.sender instead of the _registry parameter passed to the constructor. This results in the registry pointing to the address of the contract deployer rather than the intended CharityRegistry contract.

Vulnerability Details

The vulnerability is an Initialization Error located in the constructor of the GivingThanks contract. The issue arises from the incorrect assignment of the registry variable. Instead of being initialized with the _registry parameter, which is intended to be the address of a CharityRegistry contract, it is mistakenly set to msg.sender. This oversight causes the registry to point to the deployer's address rather than the correct CharityRegistry contract, leading to the malfunction of critical functionalities such as charity verification and donation processing.

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

POC

function testDonate() public {
uint256 donationAmount = 1 ether;
​
// Check initial token counter
uint256 initialTokenCounter = charityContract.tokenCounter();
​
// Fund the donor
vm.deal(donor, 10 ether);
​
// Donor donates to the charity
vm.prank(donor);
charityContract.donate{value: donationAmount}(charity);
​
// Check that the NFT was minted
uint256 newTokenCounter = charityContract.tokenCounter();
assertEq(newTokenCounter, initialTokenCounter + 1);
​
// Verify ownership of the NFT
address ownerOfToken = charityContract.ownerOf(initialTokenCounter);
assertEq(ownerOfToken, donor);
​
// Verify that the donation was sent to the charity
uint256 charityBalance = charity.balance;
assertEq(charityBalance, donationAmount);
}

Output

Test with registry = CharityRegistry(msg.sender);

Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[FAIL. Reason: EvmError: Revert] testDonate() (gas: 27751)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.75ms (544.30µs CPU time)
​
Ran 1 test suite in 63.27ms (4.75ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
​
Failing tests:
Encountered 1 failing test in test/GivingThanks.t.sol:GivingThanksTest
[FAIL. Reason: EvmError: Revert] testDonate() (gas: 27751)
​
Encountered a total of 1 failing tests, 0 tests succeeded
​

Test with registry = CharityRegistry(_registry);

Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[PASS] testDonate() (gas: 293401)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.31ms (1.54ms CPU time)
​
Ran 1 test suite in 67.92ms (7.31ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

  • The contract will not function as intended since the registry will not point to a valid CharityRegistry contract.

  • The donate function will always fail the require(registry.isVerified(charity), "Charity not verified") check, as the isVerified function will not be callable on a non-CharityRegistry address.

Tools Used

Manual Code Review and Foundry Unit Test

Recommendations

Modify the constructor to correctly initialize the registry variable with the _registry parameter:

constructor(address _registry) ERC721("DonationReceipt", "DRC") {
registry = CharityRegistry(_registry);
owner = msg.sender;
tokenCounter = 0;
}
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.