Nouns Builder contest - 0x1337's results

1. Missing 0 address check in the transferOwnership() function.

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/utils/Ownable.sol#L63-L67

There's no check that the _newOwner is not 0 address. It is dangerous and irreversible if the owner is accidentally set to be 0 address. Recommend using the OpenZeppelin implementation of this function, which contains the zero address check:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6a8d977d2248cf1c115497fccfd7a2da3f86a58f/contracts/access/Ownable.sol#L69-L72

2. Malleability issue with ecrecover()

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

EVM’s ecrecover is susceptible to signature malleability, which allows replay attacks. This is mitigated here as each recovered voter can only vote once. However, if any application logic changes, it might make signature malleability a risk for replay attacks.

See this reference: https://swcregistry.io/docs/SWC-117

Recommend using OpenZeppelin's ECDSA library: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol

3. Lack of storage gap in upgradeable contracts, especially base contracts in the inheritance chain.

None of the upgradeable contracts in the code base contains storage gap, which is not a best practice and potentially limit the ability to add new variables. This is somewhat mitigated, because most of the contracts in the code base put all the variables that take storage slot into one base contract, e.g. AuctionStorageV1.sol, and the storage contracts can be upgraded to include new variables in the future, because the contract that inherits the storage contract (e.g. Auction.sol) only has immutable or constant variables that do not take storage slots. However, there are exceptions to this. For example, the Token contract inherits the ERC721Votes contract, which in turn inherits the ERC721 contract, none of which contains storage gap. If a new variable is added to the ERC721Votes contract, it would overwrite the variable in the TokenStorageV1 contract, and if a new variable is added to the ERC721 contract, it would overwrite the variable in the ERC721Votes contract. This greatly limits the ability to upgrade base contracts. Recommend including storage gap in all the base contracts for upgradeable contracts in the code base.