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:
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.
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.
GalloDaSballo
commented
2 years ago
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
_newOwneris 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, theTokencontract inherits theERC721Votescontract, which in turn inherits theERC721contract, none of which contains storage gap. If a new variable is added to theERC721Votescontract, it would overwrite the variable in theTokenStorageV1contract, and if a new variable is added to theERC721contract, it would overwrite the variable in theERC721Votescontract. 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.