Open sherlock-admin opened 9 months ago
This is valid and is the core issue behind #247 as well. baseTokenId should start at 0 in addFounders
I initially separated the 4 findings below, but I agree, #177, #247 and #67 are only possible because of the following lines of code here, wherein _addFounder()
, baseTokenId
is incorrectly initialized to reservedUntilTokenId
in addFounders()
, which is the root cause of the issue, and once fixed, all the issues will be fixed too. There are 4 impacts mentioned by watsons.
uint256 baseTokenId = reservedUntilTokenId;
Previous founders that are meant to be deleted are retained causing them to continue receiving minted NFTs --> High severity, since it is a definite loss of funds
reserveTokenId
greater than 100 will cause a 1% loss of NFT for founder --> High severity, since it is a definite loss of funds for founder as long as reservedUntilTokenId
is set greater than 100, which is not unlikelybaseTokenId
is incorrectly set as reservedUntilTokenId
but will cause a definite loss of founders NFT if performed, so keeping as duplicatereservedUntilTokenId
via setReservedUntilTokenId
can cause over/underallocation NFTs so keeping as duplicateHowever, in the context of the audit period, I could also see why watsons separated these issues, so happy to hear from watsons during escalation period revolving deduplication of these issues.
Hi @neokry would be helpful if you could highlight to watsons here why you think the following primary issues should be duplicated under this issue:
From my understanding it stems from the _addFounders()
function used in both the initialize()
and updateFounders()
function, in particular the following line here,
uint256 baseTokenId = reservedUntilTokenId;
But it would be extremely helpful if you could provide a more detailed explanation in each finding, and show how the fix to #42 also fixes the rest of the findings.
To all watsons, this is my initial deduplication here, feel free to also provide me the flow state of the functions to prove that they do not have the same root cause.
Hi watsons,
The core of issue #42 is that baseTokenId
should not start with reservedUntilTokenId
within addFounders()
I believe this issue and its duplicates are invalid as there is a misunderstanding of how founders token amount are assigned based on this comment here
Both #177 and #247 and its duplicates
This issue hinges on the same root cause that baseTokenId
is initialized as reservedUntilTokenId
. However, the key difference here is that updateFounders()
is also affected, which is a completely different function. However, I still think that this should be duplicated with #42, based on sherlock duplication rules, more specifically, see point 1.1 and 2. The only point where they cannot be considered duplicates is when the fixes are different.
Unless a watson can prove to me that the fix implemented here by the sponsor is insufficient, I am inclined to keep all of them as duplicates except the above mentioned #67 and its duplicates.
Fix looks good. BaseTokenId now always starts at 0.
bin2chen
high
when reservedUntilTokenId > 100 first funder loss 1% NFT
Summary
The incorrect use of
baseTokenId = reservedUntilTokenId
may result in the firsttokenRecipient[]
being invalid, thus preventing the founder from obtaining this portion of the NFT.Vulnerability Detail
The current protocol adds a parameter
reservedUntilTokenId
for reservingToken
. This parameter will be used as the startingbaseTokenId
during initialization.Because
baseTokenId = reservedUntilTokenId
is used, ifreservedUntilTokenId>100
, for example, reservedUntilTokenId=200, the first_getNextTokenId(200)
will returnbaseTokenId=200 , tokenRecipient[200]=newFounder
.Example: reservedUntilTokenId = 200 founder[0].founderPct = 10
In this way, the
tokenRecipient[]
offounder
will become tokenRecipient[200].wallet = founder ( first will call _getNextTokenId(200) return 200) tokenRecipient[10].wallet = founder ( second will call _getNextTokenId((200 + 10) %100 = 10) ) tokenRecipient[20].wallet = founder ... tokenRecipient[90].wallet = founderHowever, this
tokenRecipient[200]
will never be used, because in_isForFounder()
, it will be modulo, so onlybaseTokenId < 100
is valid. In this way, the first founder can actually only9%
of NFT.POC
The following test demonstrates that
tokenRecipient[200]
is for founder.need change tokenRecipient to public , so can assertEq
add to
token.t.sol
Impact
when reservedUntilTokenId > 100 first funder loss 1% NFT
Code Snippet
https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L161
Tool used
Manual Review
Recommendation
A better is that the baseTokenId always starts from 0.
or
use
uint256 baseTokenId = reservedUntilTokenId % 100;