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

Wrong use of msg.sender in `GivingThanks:constructor` breaks protocol

trashpirate

Summary

In GivingThanks::constructor the variable msg.sender is used to register the previously deloyed instance of CharityRegistry. Instead the constructor argument _registry should be used.

Vulnerability Details

Using msg.sender to register the previously deployed contract instance of CharityRegistry means that the contract instance is not registered at the correct address. Because there is no contract instance deployed at the address msg.sender this breaks all functionality of CharityRegistry which is critical for proper functioning of the protocol.

Proof of Concept

A simple way to verify whether the correct registry address is set in GivingThanks::constructor is to test the value of registry after deployment.

Code:

The following test if placed in GivingThanks.t.sol should pass if registry is configured correctly:

```solidity
function testRegsitryAddress() public {
assertEq(address(registryContract), address(charityContract.registry()));
}
```

Impact

Protocol is broken as the CharityRegistry is not registered at the correct address.

Tools Used

Foundry, manual review

Recommendations

Replace msg.sender with _registry in GivingThanks::constructor:

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 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.