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

Unused OpenZeppelin `Ownable` Contract Import and Custom Owner Variable

4rdiii

Summary

The GivingThanks contract imports OpenZeppelin's Ownable contract but does not use it, instead opting for a custom owner variable to store the contract owner. This inconsistency may lead to unnecessary complexity and missed features from OpenZeppelin’s secure, standardized ownership management functions.

Vulnerability Details

The contract includes Ownable.sol but defines a custom owner variable without utilizing the features of the Ownable contract, such as access control functions (onlyOwner), or standardized methods (transferOwnership).

import "@openzeppelin/contracts/access/Ownable.sol";
.
.
.
contract GivingThanks is ERC721URIStorage {
CharityRegistry public registry;
uint256 public tokenCounter;
address public owner;

Impact

  • Redundant Code: Increased bytecode size due to unused Ownable import.

  • Missed Access Control: Risks bypassing well-tested access control functions and may lead to future inconsistencies in owner-based functionality.

  • Maintainability: Using Ownable’s built-in features such as transfer or renounce ownership would streamline ownership logic and improve readability for other developers.

Tools Used

Manual Review

Recommendations

Refactor the GivingThanks contract to extend Ownable directly, removing the custom owner variable. Update functions that require admin privileges to use onlyOwner for access control. For example:

import "@openzeppelin/contracts/access/Ownable.sol";
​
contract GivingThanks is ERC721URIStorage, Ownable {
CharityRegistry public registry;
uint256 public tokenCounter;
- address public owner;
​
constructor(address _registry) ERC721("DonationReceipt", "DRC") {
registry = CharityRegistry(_registry);
- owner = msg.sender;
tokenCounter = 0;
}
​
// Update all owner checks with onlyOwner modifier
function updateRegistry(address _registry) public onlyOwner {
registry = CharityRegistry(_registry);
}
}

This approach utilizes the robust, tested access control from OpenZeppelin’s Ownable contract and eliminates unnecessary custom code.

Updates

Lead Judging Commences

n0kto Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.