Nouns Builder contest - lukris02's results

QA Report for Nouns Builder contest

Overview

During the audit, 3 low and 5 non-critical issues were found.

Low Risk Findings (3)

L-1. Storage variables can be packed tightly

Description

According to docs, multiple, contiguous items that need less than 32 bytes are packed into a single storage slot if possible. It might be beneficial to use reduced-size types if you are dealing with storage values because the compiler will pack multiple elements into one storage slot, and thus, combine multiple reads or writes into a single operation.

Instances

• 1. Link to code

        uint16 proposalThresholdBps;
        uint16 quorumThresholdBps;
        Treasury treasury;
        uint48 votingDelay;
        uint48 votingPeriod;

• 2. Link to code

        uint96 totalSupply;
        IBaseMetadata metadataRenderer;
        uint8 numFounders;
        uint8 totalOwnership;

Recommendation

Consider changing order of variables to:

• 1.

        Treasury treasury;
        //PACKED
        uint16 proposalThresholdBps;
        uint16 quorumThresholdBps;
        uint48 votingDelay;
        uint48 votingPeriod;

        IBaseMetadata metadataRenderer;
        //PACKED
        uint96 totalSupply;
        uint8 numFounders;
        uint8 totalOwnership;

L-2. Large number of items may cause out-of-gas error

Description

Loops that do not have a fixed number of iterations, for example, loops that depend on storage values, have to be used carefully: Due to the block gas limit, transactions can only consume a certain amount of gas. Either explicitly or just due to normal operation, the number of iterations in a loop can grow beyond the block gas limit, which can cause the complete contract to be stalled at a certain point.

Instances

• for (uint256 i = 0; i < numTargets; ++i) {
• for (uint256 i = 0; i < numNewProperties; ++i) {
• for (uint256 i = 0; i < numNewItems; ++i) {
• for (uint256 i; i < numFounders; ++i) {

Recommendation

Restrict the maximum number of items in loop.

L-3. Empty function bodies

Instances

• function tokenURI(uint256 _tokenId) public view virtual returns (string memory) {}
• function contractURI() public view virtual returns (string memory) {}
• function _authorizeUpgrade(address _newImpl) internal override onlyOwner {}

Non-Critical Risk Findings (5)

NC-1. Public functions can be external

Description

If functions are not called by the contract where they are defined, they can be declared external.

Instances

• function onERC721Received(
• function onERC1155Received(
• function onERC1155BatchReceived(
• function tokenURI(uint256 _tokenId) public view virtual returns (string memory) {}
• function contractURI() public view virtual returns (string memory) {}
• function balanceOf(address _owner) public view returns (uint256) {
• function ownerOf(uint256 _tokenId) public view returns (address) {
• function getVotes(address _account) public view returns (uint256) {
• function getPastVotes(address _account, uint256 _timestamp) public view returns (uint256) {
• function DOMAIN_SEPARATOR() public view returns (bytes32) {
• function owner() public view returns (address) {
• function pendingOwner() public view returns (address) {
• function transferOwnership(address _newOwner) public onlyOwner {
• function safeTransferOwnership(address _newOwner) public onlyOwner {
• function acceptOwnership() public onlyPendingOwner {
• function cancelOwnershipTransfer() public onlyOwner {
• function tokenURI(uint256 _tokenId) public view override(IToken, ERC721) returns (string memory) {
• function contractURI() public view override(IToken, ERC721) returns (string memory) {

Recommendation

Make public functions external, where possible.

NC-2. Order of Functions

Description

According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private

Instances

1. receive function in wrong place
2. internal functions before public and (2)
3. public and internal functions before external and (2), (3), (4)
4. private and external functions between internal and (2), (3)
5. public functions between external and (2)
6. modifier after functions

Recommendation

Reorder functions where possible.

NC-3. Scientific notation may be used

Description

For readability, it is better to use scientific notation.

Instances

success := call(50000, _to, _amount, 0, 0, 0, 0)

Recommendation

Replace 50000 with 50e4.

NC-4. Missing NatSpec

Description

NatSpec is missing for 10 functions in 2 contracts.

Instances

• 7 functions in SafeCast.sol
• 3 functions in TokenReceiver.sol

Recommendation

Add NatSpec for all functions.

NC-5. Inconsistent use of named return variables

Description

Some functions return named variables, others return explicit values.

Instances

For example:

    /// @notice The minimum percentage an incoming bid must raise the highest bid
    function minBidIncrement() external view returns (uint256);


    /// @notice Updates the time duration of each auction
    /// @param duration The new time duration
    function setDuration(uint256 duration) external;