giraffe - First Founder unable to mint token due to tokenId incorrectly set to reservedUntilTokenId

[sherlock-admin2]

giraffe

high

First Founder unable to mint token due to tokenId incorrectly set to reservedUntilTokenId

Summary

When adding founders during Token.sol initialization, the first founder will have his first tokenId set to reservedUntilTokenId. This has two consequences 1) the first founder cannot mint his first token using mintFromReserveTo, and 2) reservedUntilTokenId will incorrectly point to a reserved token.

Vulnerability Detail

By design, thereservedUntilTokenId should be the first tokenId that the DAO's auctions will use. If it was set to 100 then it means that tokenIds 0 to 99 are reserved.

/// @param _reservedUntilTokenId The tokenId that a DAO's auctions will start at

The error lies in the function _addFounders. Unlike subsequent tokens, this first reserved token does not go through % 100 and therefore is set to be equal to reservedUntilTokenId. E.g. if reservedUntilTokenId = 100, the first founder's first reserved token will be id 100.

function _addFounders() internal {
	...	
	// @audit first founder's first tokenId set to reservedUntiltokenId
	uint256 baseTokenId = reservedUntilTokenId;	  
	
	for (uint256 j; j < founderPct; ++j) {	
		// @audit tokenId is unchanged by _getNextTokenId since token is unassigned
		baseTokenId = _getNextTokenId(baseTokenId);	
		tokenRecipient[baseTokenId] = newFounder;  
		
		emit MintScheduled(baseTokenId, founderId, newFounder);  
			
		// @audit only subsequent tokenIds have the % 100 treatment
		baseTokenId = (baseTokenId + schedule) % 100;	
	}
}

function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {
	unchecked {
		while (tokenRecipient[_tokenId].wallet != address(0)) {
		_tokenId = (++_tokenId) % 100;
		} 
		
		return _tokenId;
	}
}

Impact

The first founder is unable to mint his first token through mintFromReserveToas it would fail the check:

 if (tokenId >= reservedUntilTokenId) revert TOKEN_NOT_RESERVED();

Consider this proof-of-concept:

• Assume reservedUntilTokenId = 100
• After adding founders, it is checked that tokenId 100 belongs to the first founder
• If minter tries to mint tokenId 100 to founder, it will revert with "TOKEN_NOT_RESERVED"

Add this foundry test to Token.t.sol

function test_cannotMintFirstToken() public {	
	uint256 _reservedUntilTokenId = 100;	
	deployAltMock(_reservedUntilTokenId);	  
	
	(address wallet100,,) = token.tokenRecipient(100);	
	assertTrue(wallet100 == address(founder));	  
	
	address _minter = vm.addr(0x1234);	
	TokenTypesV2.MinterParams[] memory minters = new TokenTypesV2.MinterParams[](1);	
	TokenTypesV2.MinterParams memory p1 = TokenTypesV2.MinterParams({ minter: _minter, allowed: true });	
	minters[0] = p1;	  
	
	vm.prank(address(founder));	
	token.updateMinters(minters);  
	
	vm.startPrank(minters[0].minter);  
	
	bytes4 selector = bytes4(keccak256(abi.encodePacked("TOKEN_NOT_RESERVED()")));
	
	vm.expectRevert(selector);	
	// First founder cannot mint first tokenId	
	token.mintFromReserveTo(founder, 100);
}

Considering that the first founder is very likely an important person in the DAO, losing some ownership due to failure to mint a token is quite impactful. The likelihood is also high or even bound to occur due to the very first reserved tokenId being incorrectly set to reservedForTokenId.

Code Snippet

https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L161

Tool used

Manual Review

Recommendation

Consider changing _addFounders() from:

uint256 baseTokenId = reservedUntilTokenId;
to
`uint256 baseTokenId = reservedUntilTokenId % 100;

Duplicate of #42