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

[M-02] GivingThanks Constructor Uses msg.sender Instead of Registry Parameter

iepathos

Summary

The GivingThanks contract's constructor incorrectly initializes the registry address using msg.sender instead of the provided _registry parameter. This causes the contract to fail to interact with the intended registry unless manually fixed post-deployment.

Vulnerability Details

The vulnerability exists in the constructor:

constructor(address _registry) ERC721("DonationReceipt", "DRC") {
registry = CharityRegistry(msg.sender); // BUG: Uses msg.sender instead of _registry
owner = msg.sender;
tokenCounter = 0;
}

Key issues:

  1. Registry parameter is ignored

  2. msg.sender is used instead of _registry

  3. Forces reliance on updateRegistry() to fix post-deployment

  4. Could break core functionality if msg.sender isn't a valid registry

This causes:

  1. Contract deploys with incorrect registry address

  2. Initial donations will fail unless fixed

  3. Additional transaction required to set correct registry

Proof of Concept:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
import "../../src/CharityRegistry.sol";
import "../../src/GivingThanks.sol";
​
contract RegistryInitializationTest is Test {
CharityRegistry public registry;
GivingThanks public givingThanks;
address public donor = address(0x1);
address public charity = address(0x2);
function setUp() public {
registry = new CharityRegistry();
// Register and verify a legitimate charity
registry.registerCharity(charity);
registry.verifyCharity(charity);
// Fund donor
vm.deal(donor, 2 ether);
}
​
function testIncorrectInitialization() public {
// Deploy GivingThanks with registry address
givingThanks = new GivingThanks(address(registry));
// Attempt donation - should fail because registry is set to deployer
vm.prank(donor);
vm.expectRevert();
givingThanks.donate{value: 1 ether}(charity);
// Fix registry address
givingThanks.updateRegistry(address(registry));
// Now donation should work
vm.prank(donor);
givingThanks.donate{value: 1 ether}(charity);
// Verify donation succeeded
assertEq(charity.balance, 1 ether);
}
​
function testDeployerNotRegistry() public {
// Deploy from address that isn't a registry
vm.prank(donor);
givingThanks = new GivingThanks(address(registry));
// Try to verify charity status - will fail because donor address isn't a registry
vm.expectRevert();
givingThanks.donate{value: 1 ether}(charity);
}
}

Impact

  • Severity: Medium

  • Likelihood: High (affects every deployment)

Effects:

  1. Contract deploys in broken state

  2. Donations fail until registry is fixed

  3. Additional gas costs for fixing registry

  4. Potential for permanent failure if updateRegistry() is restricted

Tools Used

  • Foundry Test Framework

  • Manual code review

Recommendations

Fix the constructor to use the provided registry parameter:

constructor(address _registry) ERC721("DonationReceipt", "DRC") {
require(_registry != address(0), "Zero address");
registry = CharityRegistry(_registry);
owner = msg.sender;
tokenCounter = 0;
// Optional: Verify registry is valid
require(registry.isVerified(address(0)) != address(0), "Invalid registry");
}

Additionally, consider:

  1. Adding events for registry changes

  2. Adding validation that the provided address is a valid registry

  3. Protecting updateRegistry() with access control

  4. Adding ability to recover from invalid registry state

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.