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

Incorrect constructor parameter defined in `GivingThanks` contract resulting mismatch of registry information upon deployment

soupy

Summary

Constructor input parameter _registry in contract GivingThanks is not used to define the CharityRegistry causing the registry variable was not referred to the correct implementation right after contract deployment, resulting a mismatch of implementation for the correct CharityRegistry within the GivingThanks protocol.

https://github.com/Cyfrin/2024-11-giving-thanks/blob/304812abfc16df934249ecd4cd8dea38568a625d/src/GivingThanks.sol#L15-L19

Vulnerability Details

In contract GivingThanks, the input parameter _register was not used to define the CharityRegistry variable. Instead, msg.sender is used to define the variable registry. This causes the registry variable does not reflect the correct registry implementation upon contract deployment

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

Proof of Concept:

Add the following test to test\GivingThanks.t.sol

function test_audit_charityRegistryAddressAtConstructorOfGivingThanksContract() public view {
address registryAddr = address(charityContract.registry());
console.log("admin: ", admin);
console.log("address of registryContract @ deployment: ", address(registryContract));
console.log("address of registryContract @ retrival of registry variable: ", registryAdd);
​
// since charityConstract was deployed using the registryContract as its input parameter during the setUp in the test suite, the registryAdd should capture the same value as the registryContract
assertEq(registryAdd, address(registryContract));
}

Run the test forge test --match-test test_audit_charityRegistryAddressAtConstructorOfGivingThanksContract -vvv

Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[FAIL: assertion failed: 0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF != 0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395] test_audit_charityRegistryAddressAtConstructorOfGivingThanksContract() (gas: 20051)
Logs:
admin: 0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF
address of registryContract @ deployment: 0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395
address of registryContract @ retrival of registry variable: 0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF

The test failed indicating that the registry variable captured in the GivingThanks contract is different from what was defined in the constructor input parameter in the setUp function

Impact

Mismatch of registry variable information in GivingThanks contract versus the input given during contract deployment causes confusion and doubts to protocol users.

Tools Used

Manual review with tset

Recommendations

Replace the msg.sender to _register in the constructor of contract GivingThanks as follow:

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

Rerun the test above forge test --match-test test_audit_charityRegistryAddressAtConstructorOfGivingThanksContract -vvv

Ran 1 test for test/GivingThanks.t.sol:GivingThanksTest
[PASS] test_audit_charityRegistryAddressAtConstructorOfGivingThanksContract() (gas: 20047)
Logs:
admin: 0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF
address of registryContract @ deployment: 0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395
address of registryContract @ retrival of registry variable: 0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 550.38µs (53.79µs CPU time)

The test passed indicating that the recommended change had corrected the registry implementation.

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.