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

Bug Report: Code Design and Optimizations in GivingThanks.sol

koinerdegen

Summary:
The GivingThanks.sol contract needs some improvements to enhance code readability, structure, naming conventions, and performance. The following changes are proposed:

  • Rename variables to follow a consistent naming convention.

  • Reorganize functions for better readability and maintainability.

  • Add Natspec documentation for better understanding and standardization.

  • Improve imports and code layout for better modularity.

Vulnerability Details:
The contract currently lacks standardized variable naming, clear function ordering, and missing Natspec documentation. Additionally, the imports are not modular, and there is a lack of clarity in how the contract functions are arranged.

Impact:
Low Impact: The changes do not introduce any new vulnerabilities or risks but aim to enhance the readability, maintainability, and standardization of the contract code. Adopting a more modular design will make the code easier to understand, and ensuring Natspec documentation will help external developers or auditors understand the functionality.

Tools Used:

  • Manual code review

Recommendations:

  1. Renaming Variables:

    • tokenCounter should be renamed to s_tokenCounter for consistency with naming conventions.

    • owner should be renamed to i_owner to follow the convention for immutable state variables.

  2. Reordering Functions: Functions should be ordered as per best practices:

    • external functions first

    • public functions next

    • internal functions after that

    • private functions last

    • View and pure functions should be grouped separately

  3. Adding Natspec: Natspec comments should be added to all functions to enhance code documentation and make the contract more understandable.

  4. Improved Imports: Use modular import statements to improve clarity and separation of concerns.

Solution: Updated Code:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;
​
// Module Imports
import {CharityRegistry} from "./CharityRegistry.sol";
import {ERC721URIStorage} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
import {Base64} from "@openzeppelin/contracts/utils/Base64.sol";
​
/**
* @title GivingThanks
* @dev This contract allows users to donate to verified charities and receive a token of thanks (NFT) as proof of donation.
*/
contract GivingThanks is ERC721URIStorage {
// Immutable state variable for the contract owner
address public immutable i_owner;
// State variable to track the token ID counter
uint256 public s_tokenCounter;
​
// CharityRegistry contract instance
CharityRegistry public registry;
​
/**
* @dev Constructor function to initialize the contract
* @param _registry The address of the CharityRegistry contract.
*/
constructor(address _registry) ERC721("DonationReceipt", "DRC") {
registry = CharityRegistry(_registry);
i_owner = msg.sender;
s_tokenCounter = 0;
}
​
// External Functions
​
/**
* @dev Allows a user to donate to a verified charity and mint an NFT as proof of donation.
* @param charity The address of the charity to donate to.
*/
function donate(address charity) external payable {
require(registry.isVerified(charity), "Charity not verified");
​
// Send donation to charity
(bool sent, ) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
​
// Mint an NFT for the donor
_mint(msg.sender, s_tokenCounter);
​
// Create metadata for the token URI
string memory uri = _createTokenURI(
msg.sender,
block.timestamp,
msg.value
);
​
// Set the token URI
_setTokenURI(s_tokenCounter, uri);
​
// Increment token counter
s_tokenCounter += 1;
}
​
// Public Functions
​
/**
* @dev Allows the owner to update the registry address.
* @param _registry The new address of the CharityRegistry contract.
*/
function updateRegistry(address _registry) public {
registry = CharityRegistry(_registry);
}
​
// Internal Functions
​
/**
* @dev Internal function to create the token URI metadata.
* @param donor The address of the donor.
* @param date The timestamp of the donation.
* @param amount The amount donated.
* @return A string representing the token URI (base64 encoded JSON).
*/
function _createTokenURI(
address donor,
uint256 date,
uint256 amount
) internal pure returns (string memory) {
// Create JSON metadata
string memory json = string(
abi.encodePacked(
'{"donor":"',
Strings.toHexString(uint160(donor), 20),
'","date":"',
Strings.toString(date),
'","amount":"',
Strings.toString(amount),
'"}'
)
);
​
// Encode in base64 using OpenZeppelin's Base64 library
string memory base64Json = Base64.encode(bytes(json));
​
// Return the data URL
return
string(
abi.encodePacked("data:application/json;base64,", base64Json)
);
}
​
// Private Functions
​
// No private functions in this contract
}
​

2. Explanation of Changes:

Renaming Variables:

  • tokenCounter is renamed to s_tokenCounter to align with the standard for state variables that are not immutable.

  • owner is renamed to i_owner to reflect its immutable nature.

Reorganizing Functions:

  • The functions are now grouped as follows:

    • External Functions: Functions that are intended to be called from outside the contract, such as donate.

    • Public Functions: Functions that are callable externally but also from within the contract (e.g., updateRegistry).

    • Internal Functions: Functions that can only be called within the contract, such as _createTokenURI.

    • Private Functions: No private functions were required in this case, so the section is omitted.

Adding Natspec:

  • Natspec comments have been added to all functions, explaining their purpose, parameters, and expected behavior. This improves code clarity and helps with external audits and understanding of the contract.

Improved Imports:

  • The import statements have been updated to use modular imports:

    • import {CharityRegistry} from "./CharityRegistry.sol";

    • import {ERC721URIStorage} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";

    • import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

    • import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";

    • import {Base64} from "@openzeppelin/contracts/utils/Base64.sol";

This improves clarity and ensures each contract is imported properly without any unnecessary dependencies.

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.