GivingThanks

Summary:

1. The happen to be an unused variable in the constructor parameter _registry() that is not properly utilised within the constructor logic which can cause potential conflicts.

2. Constructor Inconsistency:
   The constructor passes msg.sender to both ERC721 initializer and owner field. The owner fields redundant due to openzeppelin ownable contract which already manages the owner.

Vulnerability Details:

https://github.com/Cyfrin/2024-11-giving-thanks/blob/main/src/GivingThanks.sol

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

Impact:

The presence of unused variables may lead to unnecessary errors and warnings making the code less readable.

Constructor Inconsistency: This may lead to conflicts in ownership management if the contract owner changes through ownable functions.

Tools Used:

Manual review

Recommendations:

1. Make sure the _registry is initialized or remove it from the constructor parameter.

2. The ownable contract already handles owner management, so we can remove the address public owner declaration and assignment.