Nouns Builder contest - leosathya's results

[L-01] IMMUTABLE ADDRESSES LACK ZERO-ADDRESS CHECK

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

There is 10 instance of this issue:

File : Token.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L30-L31

File : Manager.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L62-L66

File : Treasury.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L33

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L41-L43

File : Auction.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L40-L41

Mitigation

check for zero address

[L-02] ADDRESSES LACK ZERO-ADDRESS CHECK BEFORE ASSIGNING THEM TO STATE VARIABLE

Should check the addresses before assigning them to state variables

There is 2 instance of this issue:

File : Token.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L65-L66

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L41-L43

File : Auction.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L79

Mitigation

check for zero address

[L-03] _addFounders() IN Token.sol MAY LEAD TO DoS(block gas limit exceed) CONDITION

_addFounder() used Nested For loops on Dynamic Array inside which forther internal function calls occur which makes state change, This is quite gas consuming, if length of Dynamic Array will too long, then it leads to Block gas limit exceed condition.

There is 1 instance of this issue:

File : Token.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L71-L125

Mitigation

. Use Small size array as Input . Make a Boundary for input array length

[L-04] _SAFEMINT() SHOULD BE USED RATHER THAN _MINT() WHEREVER POSSIBLE

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both OpenZeppelin and solmate have versions of this function

There is 1 instance of this issue:

File : Token.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L161

Mitigation

Use SafeMint function.

[L-05] _authorizeUpgrade() OF Manager.sol SHOULD DO SOMETHING, ATLEAST EMIT SOME EVENTS WHEN NEW IMPLEMENTED ADDRESS CHANGE

_authorizeUpgrade() function is just blank block which doing nothing, At least it should emit a events when Implemented address changes

File : Manager.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L209-L210

[L-06] execute() FROM Treasury.sol DOESN'T CHECK FOR DYNAMIC INPUT ARRAY LENGTH

3 dynamic array are take as input parameter and then used in a For loop, There is no code syntax for checking 3 arrays are of same length or not. As a Human error Owner can copy paste array of different length that ultimately result in function fail.

File : Treasury.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L141-L172

Mitigation

Should have condition to check that all arrays have same length.

[L-07] CONSIDER ADDINGS CHECKS FOR SIGNATURE MALLEABILITY

Use OpenZeppelin’s ECDSA contract rather than calling ecrecover() directly

There is 1 instance of this issue:

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

[L-08] LACK OF UINT VARIIABLE CHECK

uint check absent for _reservePrice, during initialization in Auction contract, which can be set to 0 at the time of initialization

File : Auction.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L78

[L-09] CRITICAL SETTINGS CHANGED BY OWNER ANYTIME

In Auction contract critical settings (Important variables regarding auction) can be changed by owner anytime using functions like setDuration(), setReservePrice(), setTimeBuffer(), setMinimumBidIncrement()

File : Auction.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L307-L311 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L315-L319 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L323-L327 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L331-L335

Mitigation

There should be some governace system with a Timelock feature, that gives Audience to make their decisions

[L-09] OWNER CAN INCREMENT BIDDING PRICE ANYTIME

No check presents for input parameter percentage in function setMinimumBidIncrement(), that could be Any orbitary amount. This could lead to a Front-Running case,

File : Auction.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L331-L335

Mitigation

There should be a Timelock feature for changing critical feature like this, that gives Audience to make their decisions

[L-10] NO CHECK FOR RETURN VALUE OF IWETH TRANSFER

Their is no check for return value of IWETH.transfer that could lead to the Eth loss for bidder in Auction contract.

File : Auction.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L363

Mitigation

There should be a return value check present for IWETH.transfer More important here Contract using Pushing instaed of Pulling which can lead to DoS if attacker intend to so, and he will lose his fund

[G-01] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

There is 4 instance of this issue:

File : Token.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L88

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#280 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#285 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#290

Mitigation

for example ::

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

To

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

[G-02] >= COSTS LESS GAS THAN >

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas

There is 5 instance of this issue:

File : Auction.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L106 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L122 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L141 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L346

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#128

[G-03] VARIABLE DECLARED AND THEN INITIALIZED

Here "founder" variable first initialized to its default value, then after again changed to other value and checked in if condition

File : Manager.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L114-L117

Mitigation

address founder; if ((founder = _founderParams[0].wallet) == address(0)) revert FOUNDER_REQUIRED();

To

address founder = _founderParams[0].wallet; if ((founder) == address(0)) revert FOUNDER_REQUIRED();

[G-04] USING CALLDATA INSTEAD OF MEMORY FOR READ-ONLY ARGUMENT IN EXTERNAL FUNCTIONS CAN SAVE GAS

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L195 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L117-L120

[G-05] USING BOOLS FOR STORAGE INCURS OVERHEAD

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Asset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L264

Use uint256 in hasVoted mapping instead bool

[G-06] REPEATING SAME CODE IN DIFFERENT CONTRACTS

hashProposal() code repeated in 2 contract files

File : Treasury.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L98-L105

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L101-L108

Mitigation

shift these code of hashProposal() to a separate library, and this library in above contract files

[G-07] EMPTY BLOCKS SHOULD BE REMOVED OR EMIT SOMETHING

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

_authorizeUpgrade() function is just blank block which doing nothing, At least it should emit a events when Implemented address changes

File : Manager.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L209-L210

[G-8] 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.

File : Governer.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#564 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#572 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#580 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#588 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#596 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#605

File : Treasury.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L98-L116 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L98-L180

File : Manager.sol https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L187 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L196 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L209