GivingThanks

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

Incorrect Initialization of registry Address in GivingThanks Contract

Summary

The GivingThanks contract incorrectly assigns the deployer's address (msg.sender) to the registry state variable in the constructor, rather than using the address passed as the _registry argument. This results in the contract referencing the wrong CharityRegistry contract, which can cause it to malfunction and potentially fail in interacting with the correct registry.

Vulnerability Details

In the GivingThanks.sol contract constructor:

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

The contract attempts to assign the msg.sender address to the registry state variable. However, the intention is to pass a specific CharityRegistry contract address during deployment via the _registry parameter, not use the deployer's address.

Problem:

  • The GivingThanks contract’s constructor is mistakenly using msg.sender (the address deploying the GivingThanks contract) instead of the _registry argument.

  • This results in the GivingThanks contract pointing to the deployer's address, not the actual CharityRegistry contract.

  • As a result, functions depending on this incorrect reference will fail to interact with the intended CharityRegistry.

Impact

  • Operational Impact: The contract will not interact with the intended CharityRegistry, leading to failure in executing charity-related functions.

  • Security Risk: Unauthorized contracts could be referenced if msg.sender is used inappropriately, leading to potential privilege escalation or malicious actions.

  • User Trust: Users may lose confidence in the system if the contract fails to operate as expected.

Tools Used

Manual Code Review: Identified the issue through examination of the constructor logic.

Recommendations

To resolve this issue, the constructor should correctly assign the _registry argument to the registry state variable, as follows:

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

Severity: High

This is a high-severity issue because it disrupts critical functionality by causing the contract to interact with the wrong registry address, leading to failures in essential operations. Furthermore, it introduces potential security risks by referencing an unintended address, which could be exploited for malicious purposes.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year 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.