Nouns Builder contest - minhtrng's results

Lines of code

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

Vulnerability details

Impact

The baseTokenIds are intended to be values from 0 to 99 and each founder is meant to be assigned a number of baseTokenIds that is equal to his ownership percentage. Due to missing or false use of the modulo operator, the value of a baseTokenId can be larger than 99 which causes a founder to get less tokens minted than his ownership percentage would entitle him to.

Proof of Concept

Token.sol, L. 108-119:

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;
}

This code is intended to determine baseTokenIds, which are later used in Token._isForFounder to determine if a tokenId is supposed to be minted for a founder:

uint256 baseTokenId = _tokenId % 100;

if (tokenRecipient[baseTokenId].wallet == address(0)) {
	return false;

Due to the "% 100" operation a founder is only eligible to receive his tokens if the baseTokenIds assigned to him are in the range of 0 to 99. This is not guaranteed by the current calculation of the baseTokenIds.

Assuming there are 2 founders with ownership percentage of 17 and 33 respectively (found by testing arbitrary numbers, there are other combinations that work as well), then the second founder would be assigned two baseTokenIds that are outside of this range (102 and 105), hence he would only be minted 31% of the total tokens minted. This can be easily tested by running the existing test suite while changing the ownership percentages in NounsBuilderTest.sol and adding a console.log statement after Token.sol L. 110.

This is due to the false or missing use of modulo operators:

(baseTokenId += schedule) % 100; //L. 118

Here the schedule is added on top of the baseTokenId but the modulo does nothing

while (tokenRecipient[_tokenId].wallet != address(0)) ++_tokenId; //L. 132

The result of this can be greater than 99 and is used as baseTokenId in L. 113 without further modification.

Tools Used

Foundry tests

Recommended Mitigation Steps

Change (baseTokenId += schedule) % 100; to baseTokenId = (baseTokenId + schedule) % 100

Change while (tokenRecipient[_tokenId].wallet != address(0)) ++_tokenId; to

while (tokenRecipient[_tokenId].wallet != address(0)){
    ++_tokenId;
   if(_tokenId >= 100){
       _tokenId = _tokenId % 100;
   }
}

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L80-L99 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L124 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L251-L265

Vulnerability details

Impact

During initialization of Token.sol (more concrete during Token._addFounders), data of founders who have zero ownership percentage (which can be a reasonable case, if someone wants to be an honorary founder without compensation), is not stored correctly. The external function Token.getFounders will return the right number of structs, but there will be empty ones for each founder with zero ownership percentage. This could potentially affect future use cases that require the founders wallet data, which a DAO or the founders want to implement.

Proof of Concept

Token.sol, L. 85:

if (founderPct == 0) continue;

This skips the initialization and storage of founder data that follows afterwards:

Token.sol, L. 94-99:

Founder storage newFounder = founder[founderId];

// Store the founder's vesting details
newFounder.wallet = _founders[i].wallet;
newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
newFounder.ownershipPct = uint8(founderPct);

Token.sol, L. 124:

settings.numFounders = uint8(numFounders);

This stores the correct number of founders and is used in Token.sol, L. 251-265 to retrieve the founders data:

function getFounders() external view returns (Founder[] memory) {
	// Cache the number of founders
	uint256 numFounders = settings.numFounders;

	// Get a temporary array to hold all founders
	Founder[] memory founders = new Founder[](numFounders);

	// Cannot realistically overflow
	unchecked {
		// Add each founder to the array
		for (uint256 i; i < numFounders; ++i) founders[i] = founder[i];
	}

	return founders;
}

Tools Used

The POC has been verified by modifying the existing test cases and retrieving the founders data.

Recommended Mitigation Steps

Move the line if (founderPct == 0) continue; after L. 99 to ensure founder data is stored.