Nouns Builder contest - imare's results

Lines of code

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

Vulnerability details

Impact

From the documentation https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/docs/protocol-docs.md?plain=1#L121

But a valid/minted NFT token without previously added attributes will revert calling:

[https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L221](function tokenURI(uint256 _tokenId))

Because src/token/metadata/MetadataRenderer.sol getAttributes uses the number of attributes as a signal of minted token. In case number of attributes is zero than the call will revert with TOKEN_NOT_MINTED That happens even if the tokens is minted and successfully sold at an auction.

Mitigation:

Two solutions:

• Simple one: Explain in the documentation this behavior. What happens if no attribute is added.
• Or more complex solution: in MetadataRenderer::OnMinted(_tokenId) automatically add at least one attribute if there are no attributes in the first place. This will be more gas expensive because if the token is not sold and in the end is simply burned there is an additional gas cost cost of creating a default (onMinted) attribute.

[L-01] If auction is paused you can still bid

Impact This behaviour can draw away users because it resembles a possible rugpull

Recommended Mitigation Steps Two posibilities:

• Use of whenNotPaused modifier on Auction#createBid call https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L90

• Stop the process of biding in the application UX/frontend if the auction is paused. And save a little on transaction gas

[L-02] Immutable addresses lack zero-address check

Constructors should check the address written in an immutable address variable is not the zero address

Proof of Concept

Instances include:

Auction.sol

manager = IManager(_manager);

later the address of manager is again checked as:

if (msg.sender != address(manager)) revert ONLY_MANAGER();

But manager can be mistakenly assigned address(0)

Mitigation

Add a zero address check for manager.

[L-03] USE SAFETRANSFERFROM INSTEAD OF TRANSFERFROM

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible. Even for ERC721 tokens.

https://docs.openzeppelin.com/contracts/2.x/api/token/erc721

Recommended Mitigation Steps Mybe insted of direct sending the NTF when the auction is settled implement a user withdrawal pattern. Instead of sending use a map to identify winning addresses. If withdrawal is succesfull delete the queried entrie.

mapping(uint256 => address) internal tokenWinners;

[L-04] SIGNATURE MALLEABILITY OF EVM’S ECRECOVER

EVM’s ecrecover is susceptible to signature malleability and is recommend using OpenZeppelin’s ECDSA library.

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

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

[G-01] Use already cached auction variable

On line https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L172 instead of

        if (auction.settled) revert AUCTION_SETTLED();

use previously cached _auction value insted of storage variable read:

        if (_auction.settled) revert AUCTION_SETTLED();

[G-02] > 0 IS LESS EFFICIENT THAN != 0 FOR UNSIGNED INTEGERS

On line: https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L203

instead of

if (_from != _to && _amount > 0) {

use

if (_from != _to && _amount != 0) {

[G-03] FUNCTIONS GUARANTEED TO REVERT WHEN CALLED BY NORMAL USERS CAN BE MARKED PAYABLE

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 28 instances of this issue.