Nouns Builder contest - peritoflores's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L118

Vulnerability details

[H1] Incorrect baseTokenId calculation

LaC

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L118

Impact

baseTokenId can be 100 or more. It is not clearly explained in the documentation but I believe, for the logic, this is not a expected behaviour.
Founders are recipients of some of the first 100 tokenId.

Proof of Concept

The following logic that updates baseTokenId will not include the module in the update.

                  `  (baseTokenId += schedule) % 100`;

Recommended

[-] (baseTokenId += schedule) % 100`;
[+] baseTokenId= (baseTokendId + schedule) % 100`;

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L82 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L108

Vulnerability details

Proof of Concept

When you call _addfounders() you pass FounderParams[].

The variable foundersPct is defined as a uint256

   uint256 founderPct = _founders[i].ownershipPct;

This variable is unsafely casted to calculate that the totalOwnership is never more that 100 percent

    if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

Then is ok when you assign

            newFounder.ownershipPct = uint8(founderPct);

However, when you enter the loop to schedule minting you dont limit the loop to the casted value

That means for example if you set founderPct to 256 then it is stored as ownershipPct = 1. But the founder will be scheduling 256 tokens.

for (uint256 j; j < founderPct; ++j) {@audit high
                // Get the available token id
               baseTokenId = _getNextTokenId(baseTokenId);
                // Store the founder as the recipient
                tokenRecipient[baseTokenId] = newFounder;
                emit MintScheduled(baseTokenId, founderId, newFounder);
                // Update the base token id
                (baseTokenId += schedule) % 100;
            }   

Recommended

It is better to do one safecast at the begining

[-] uint256 founderPct = _founders[i].ownershipPct;
[+] uint8 founderPct = SafeCast.toUint8(_founders[i].ownershipPct);

Even better, I believe, is just to change the variable length is the definition of the struct FounderParams

[-] uint256 ownershipPct;
[+] uint8 ownershipPct;

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/storage/GovernorStorageV1.sol#L9

Vulnerability details

Impact

Upgradeable contracts do not follow the conventions so upgradeability is less flexible.

Proof of Concept

Storage gaps are a convention so removing them means that you will not be able to add new variables in future upgrades, at least no in your parent contracts. For your final contract it is better to add the gap in case you decide to inherit them in the future.

Gaps are missing in the following contracts.

• Pausable
• Ownable
• ReentrancyGuard
• EIP712
• TokenStorageV1
• TokenStorageTypes
• ManagerStorageV1
• GovernorStorageV1
• TreasuryStorageV1
• AuctionStorageV1

Recommended

​ Add this line at the end of every upgradeable contract

[+] uint256[50] private __gap;