GivingThanks

Summary

Zero address checks for the address state variables admin and registry are missing. This could lead to a situation where admin functions cannot be accessed anymore.

3 Found Instances:

• Found in src/CharityRegistry.sol Line: 29

  function changeAdmin(address newAdmin) public {

  require(msg.sender == admin, "Only admin can change admin");

  @> admin = newAdmin;

  }

• Found in src/GivingThanks.sol Line: 16

  constructor(address _registry) ERC721("DonationReceipt", "DRC") {

  @> registry = CharityRegistry(msg.sender);

  owner = msg.sender;

  tokenCounter = 0;

  }

• Found in src/GivingThanks.sol Line: 56

  function updateRegistry(address _registry) public {

  @> registry = CharityRegistry(_registry);

  }

Vulnerability Details

The address state variables for admin and registry are not checked for zero address in the functions CharityRegistry::verifyCharity and CharityRegistry::changeAdmin allowing the admin/user to accidentally set the state variables to the zero address.

Proof of Concept

1. Admin accidentally sets the admin address to zero address

2. Unverified charity registeredCharities

3. Admin tries to verify charity but fails

4. Admin tries to change admin address but fails

Code:

The following test demonstrates that after setting the admin to the zero address the CharityRegistry::verifyCharity function and CharityRegistry::changeAdmin function are not accessible anymore.

Impact

If not checked for zero address, the admin address or registry address could accidentally be set to the zero address. In case for the registry, this might not be critical as the registry can be updated afterwards. However, for the admin address, this could lead to a situation where admin functions are not accessible anymore without the ability to fix it by calling the CharityRegistry::changeAdmin.

Tools Used

Foundry, Aderyn, Slither, manual review

Recommendations

Check for address(0) when assigning values to address state variables. For example: