Nouns Builder contest - wagmi's results

Lines of code

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

Vulnerability details

Impact

Founders received less token supply than their ownership percentage

In Token._isForFounder() function, it checked if this token id is belonged to some founder by taking the remainder when dividing to 100 and checked with tokenRecipient[] mapping.

uint256 baseTokenId = _tokenId % 100;

That means if a founder received a scheduled with baseTokenId >= 100, he will never receive that share during vesting.

It is possible due to function _getNextTokenId(). This function is used to finds the next available base token id for a founder. It keeps increasing value by 1 unit until it found the available base token id but forget to take the modulo when the value bigger than 100.

Proof of Concept

In Token._addFounders(), loop in line 108 - 119 assigned tokenRecipient[] for each founder.

for (uint256 j; j < founderPct; ++j) {
    // 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;
}

Consider the scenario

1. Last baseTokenId = 99 and it is assigned to another founder.
2. Use _getNextTokenId() to get next baseTokenId. And it returned 100 (since 99 is not available)
3. Function _isForFounder() only take into accounted base token id < 100, so founder will never receive his share.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider take the modulo when baseTokenId >= 100 in _getNextTokenId() function.

Lines of code

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

Vulnerability details

Impact

Founder can own 100% of token supply and settings.totalOwnership value can be manipulated.

In Token._addFounders() function, line 88 made sure that total ownership of founders will never be bigger than 100%.

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

But because of unsafe cast uint8(founderPct), there is a case that founderPct > 100 but the check is still passed.

In that case founderPct > 100, that founder may even own more than 100% and all token supply will be minted to him during vesting because [line 108] use founderPct in a loop to update the mint schedule

for (uint256 j; j < founderPct; ++j) {

Proof of Concept

Consider the scenario

uint256 founderPct = 0x01201 = 4609;
uint8(founderPct) = 0x01 = 1;

Line 102 calculate the schedule

uint256 schedule = 100 / founderPct = 100 / 4609 = 0;

And loop to update the mint schedule still use the original founderPct value (not casted to uint8 yet)

for (uint256 j; j < founderPct; ++j) {
    // 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;
}

Since schedule = 0, value of baseTokenId will keep increasing by 1 unit each time and all id from 0 to 99 (which mean 100% token supply) will be owned by founder.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider use casted value (uint8) to do other calculation after checking total ownership or use SafeCast library

Summary

1. Should use already cached variable to save gas instead of calling function

In propose() function, the value of currentProposalThreshold is already get from proposalThreshold() and cached but when checking votes it calls proposalThreshold() which is unnessary and cost more gas

uint256 currentProposalThreshold = proposalThreshold();

// Cannot realistically underflow and `getVotes` would revert
unchecked {
    // Ensure the caller's voting weight is greater than or equal to the threshold
    if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
}

Should use already cached one currentProposalThreshold to do the check

Affected Codes

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

2. No need condition in the last else block

In castVote() function, since _support > 2 will be reverted already, we do not need to check _support == 2 in the last else block. Because the value of _support is always equaled to 2 after previous checks.

Affected Codes

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