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

Incorrect Casting of `msg.sender` as `CharityRegistry` Contract Breaks Functionality of the `GivingThanks` Contract

0xodus

Summary

Casting msg.sender as the CharityRegistry contract address breaks the functionality of the GivingThanks contract. Since msg.sender is not a CharityRegistry contract, the GivingThanks contract cannot access necessary functionalities such as verifying the registration and verification of charity organizations.

Vulnerability Details

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

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

  1. Constructor incorrectly casts msg.sender as the CharityRegistry contract. Therefore breaking the functionality of GivingThanks contract

POC

  • This is a test to show that the donate function will fail.

  • Copy the following function into GivingThanks.t.sol

function testDonateFails() public {
uint256 donationAmount = 1 ether;
​
// Fund the donor
vm.deal(donor, 10 ether);
​
// Donor donates to the charity
vm.prank(donor);
vm.expectRevert();
charityContract.donate{value: donationAmount}(charity);
}
​

Impact

  • This incorrect casting renders the GivingThanks contract unusable for its intended purpose.

Tools Used

Manual Review

Recommendations

Update the constructor to correctly initialize the registry with the address of an existing CharityRegistry contract passed as _registry

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