GivingThanks

Summary

The updateRegistry function in the GivingThanks contract lacks proper access control, allowing anyone to change the registry address to an arbitrary contract. This vulnerability enables attackers to redirect donations to malicious addresses by replacing the registry with a contract that falsely verifies any address. As a result, unverified and potentially malicious charities can receive donations, compromising the integrity of the platform.

Vulnerability Details

Affected Code:

Issue:

• The updateRegistry function is publicly accessible and lacks access control modifiers.

• Any user can call updateRegistry to set the registry to a contract they control.

• By replacing the registry with a malicious contract that always returns true for isVerified, attackers can bypass the charity verification process.

Proof of Concept:

Note: Before running the test, ensure that the constructor in the GivingThanks contract is corrected to properly initialize the registry variable. Update the constructor on line 16 as follows:

Test Code:

Explanation:

• Constructor Correction: The GivingThanks contract's constructor is fixed to properly set the registry variable.

• Attack Steps:

  • The attacker deploys a MaliciousRegistry contract that always returns true for isVerified.

  • The attacker calls updateRegistry to replace the legitimate registry with the malicious one.

  • A donor attempts to donate to an unverified and unregistered charity (newCharity), which is accepted due to the malicious registry.

  • The donate function proceeds, and the unverified newCharity receives the donation.

Impact

• Bypass of Verification Process: Attackers can approve any address as a verified charity.

• Financial Loss: Donations can be redirected to malicious addresses, resulting in loss of funds intended for legitimate charities.

• Erosion of Trust: The platform's integrity is compromised, leading to reputational damage and loss of user trust.

Tools Used

• Forge (Foundry): For writing and executing the test case.

• Manual Code Review: To identify the lack of access control in updateRegistry.

• Solidity Documentation: Understanding of access control patterns and best practices.

Recommendations

• Restrict Access to updateRegistry:

  Implement access control to ensure only authorized users can call updateRegistry. For example, use the onlyOwner modifier from OpenZeppelin's Ownable contract:

  import "@openzeppelin/contracts/access/Ownable.sol";

  ​

  contract GivingThanks is ERC721URIStorage, Ownable {

  // ...

  ​

  function updateRegistry(address _registry) public onlyOwner {

  registry = CharityRegistry(_registry);

  }

  }

• Set Ownership Properly:

  • Ensure that the contract owner is correctly set during deployment (typically to the deployer's address).

  • Provide mechanisms to transfer ownership securely if needed.

• Implement Events for Registry Updates:

  • Emit an event when the registry is updated to provide transparency and allow monitoring.

  event RegistryUpdated(address indexed previousRegistry, address indexed newRegistry);

  ​

  function updateRegistry(address _registry) public onlyOwner {

  emit RegistryUpdated(address(registry), _registry);

  registry = CharityRegistry(_registry);

  }

• Audit Access Control Across All Functions:

  • Review all functions in the contract to ensure appropriate access controls are in place.

  • Consider using modifiers to centralize access control logic.

By implementing these recommendations, the updateRegistry function will be secured, preventing unauthorized users from replacing the registry with malicious contracts. This ensures that only verified charities can receive donations, maintaining the integrity and trustworthiness of the platform.