dany.armstrong90 - The first founder is to be allocated less tokens than other founders.

[sherlock-admin2]

dany.armstrong90

medium

The first founder is to be allocated less tokens than other founders.

Summary

Due to computational errors, the first founder is allocated less tokens than the other founders.

Vulnerability Detail

Token.sol#_addFounders function is as follows.

File: Token.sol
120:     function _addFounders(IManager.FounderParams[] calldata _founders, uint256 reservedUntilTokenId) internal {
121:         // Used to store the total percent ownership among the founders
122:         uint256 totalOwnership;
123: 
124:         uint8 numFoundersAdded = 0;
125: 
126:         unchecked {
127:             // For each founder:
128:             for (uint256 i; i < _founders.length; ++i) {
129:                 // Cache the percent ownership
130:                 uint256 founderPct = _founders[i].ownershipPct;
131: 
132:                 // Continue if no ownership is specified
133:                 if (founderPct == 0) {
134:                     continue;
135:                 }
136: 
137:                 // Update the total ownership and ensure it's valid
138:                 totalOwnership += founderPct;
139: 
140:                 // Check that founders own less than 100% of tokens
141:                 if (totalOwnership > 99) {
142:                     revert INVALID_FOUNDER_OWNERSHIP();
143:                 }
144: 
145:                 // Compute the founder's id
146:                 uint256 founderId = numFoundersAdded++;
147: 
148:                 // Get the pointer to store the founder
149:                 Founder storage newFounder = founder[founderId];
150: 
151:                 // Store the founder's vesting details
152:                 newFounder.wallet = _founders[i].wallet;
153:                 newFounder.vestExpiry = uint32(_founders[i].vestExpiry);
154:                 // Total ownership cannot be above 100 so this fits safely in uint8
155:                 newFounder.ownershipPct = uint8(founderPct);
156: 
157:                 // Compute the vesting schedule
158:                 uint256 schedule = 100 / founderPct;
159: 
160:                 // Used to store the base token id the founder will recieve
161:                 uint256 baseTokenId = reservedUntilTokenId;
162: 
163:                 // For each token to vest:
164:                 for (uint256 j; j < founderPct; ++j) {
165:                     // Get the available token id
166:                     baseTokenId = _getNextTokenId(baseTokenId);
167: 
168:                     // Store the founder as the recipient
169:                     tokenRecipient[baseTokenId] = newFounder;
170: 
171:                     emit MintScheduled(baseTokenId, founderId, newFounder);
172: 
173:                     // Update the base token id
174:                     baseTokenId = (baseTokenId + schedule) % 100;
175:                 }
176:             }
177: 
178:             // Store the founders' details
179:             settings.totalOwnership = uint8(totalOwnership);
180:             settings.numFounders = numFoundersAdded;
181:         }
182:     }

baseTokenId is initialized to reservedUntilTokenId and then it's value is determined by _getNextTokenId function in line 166.
The code of Token.sol#_getNextTokenId function is the following.

File: Token.sol
186:     function _getNextTokenId(uint256 _tokenId) internal view returns (uint256) {
187:         unchecked {
188:             while (tokenRecipient[_tokenId].wallet != address(0)) {
189:                 _tokenId = (++_tokenId) % 100;
190:             }
191: 
192:             return _tokenId;
193:         }
194:     }

At the start time, the condition of line 188 does not hold. Thus the first baseTokenId value of the first founder will be equal to reservedUntilTokenId.
So the first baseTokenId value of the first founder will be over 100 when reservedUntilTokenId >= 100.
On the other hand, Token.sol#_isForFounder function is as follows.

File: Token.sol
263:     function _isForFounder(uint256 _tokenId) private returns (bool) {
264:         // Get the base token id
265:         uint256 baseTokenId = _tokenId % 100;
266: 
267:         // If there is no scheduled recipient:
268:         if (tokenRecipient[baseTokenId].wallet == address(0)) {
269:             return false;
270: 
271:             // Else if the founder is still vesting:
272:         } else if (block.timestamp < tokenRecipient[baseTokenId].vestExpiry) {
273:             // Mint the token to the founder
274:             _mint(tokenRecipient[baseTokenId].wallet, _tokenId);
275: 
276:             return true;
277: 
278:             // Else the founder has finished vesting:
279:         } else {
280:             // Remove them from future lookups
281:             delete tokenRecipient[baseTokenId];
282: 
283:             return false;
284:         }
285:     }

From above, baseTokenId < 100 always hold.
Thus when reservedUntilTokenId >= 100, the first founder is to be allocated less tokens than other founders.

Example:

1. Suppose that _founders.length == 2, _founders[0].ownershipPct = 2, _founders[1].ownershipPct = 2 and reservedUntilTokenId == 100.
2. Then, as a result of _addFounders function, it is hold that tokenRecipient[100] = tokenRecipient[50] = _founders[0] and tokenRecipient[1] = tokenRecipient[51] = _founders[1].
3. Since 0 <= baseTokenId < 100 holds in _isForFounder function, tokenRecipient[100] will never be used and thus _founder[0] will be allocated tokens as half of the _founder[1].

Impact

The first founder is to be allocated less tokens than other founders.
In particular, when ownershipPct == 1 of the first founder, he will not be allocated a token at all.

Code Snippet

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

Tool used

Manual Review

Recommendation

Modify Token.sol#L161 as follows.

-               uint256 baseTokenId = reservedUntilTokenId;
+               uint256 baseTokenId = reservedUntilTokenId % 100;

Duplicate of #42