Uploaded files for judging

sherlock-audit · Dec 1, 2023 · c69a147 · c69a147
1 parent c679c9c
commit c69a147
Show file tree

Hide file tree

Showing 327 changed files with 25,487 additions and 10 deletions.
diff --git a/.gitignore b/.gitignore
diff --git a/001/003.md b/001/003.md
@@ -0,0 +1,78 @@
+Uneven Clay Rook
+
+medium
+
+# Token.updateFounders will not clear tokenRecipient correctly
+
+## Summary
+Token.updateFounders function doesn't initialize `baseTokenId` as `reservedUntilTokenId` during position clearing for the owner. Because of incorrect offset it will be impossible to clear positions correctly, which leads to bigger amount of tokens for same owner.
+## Vulnerability Detail
+When new Token is created, then `_addFounders` [is called](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L92).
+
+This function loops through the founders and fetches their percentage of minted tokens during vesting. Then function populates `tokenRecipient` mapping to mark token ids that should belong to owner(tokenId % 100).
+https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L164-L175
+```solidity
+                uint256 baseTokenId = reservedUntilTokenId;
+                
+                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 = (baseTokenId + schedule) % 100;
+                }
+```
+In case if owner has 20% ownership that means that he will receive 20 out of 100 minted tokens during the vesting.
+One thing that we should note here is that `baseTokenId` is initialized with `reservedUntilTokenId` which is the amount of tokens that are reserved. As result `tokenRecipient` values will be shifted by `reservedUntilTokenId`.
+
+Using `updateFounders` function owner have ability to update founders.
+First thing function is trying to clear `tokenRecipient` mapping, so all old owner's vesting positions are cleared.
+
+https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L412-L427
+```solidity
+                uint256 baseTokenId;
+
+                for (uint256 j; j < cachedFounder.ownershipPct; ++j) {
+                    // Get the next index that hasn't already been cleared
+                    while (clearedTokenIds[baseTokenId] != false) {
+                        baseTokenId = (++baseTokenId) % 100;
+                    }
+
+                    delete tokenRecipient[baseTokenId];
+                    clearedTokenIds[baseTokenId] = true;
+
+                    emit MintUnscheduled(baseTokenId, i, cachedFounder);
+
+                    // Update the base token id
+                    baseTokenId = (baseTokenId + schedule) % 100;
+                }
+``` 
+This function is trying to clear all positions and mark them as cleared.
+This code is really similar to the one that is used in `_addFounders` function, however `baseTokenId` is 0 here and is not initialized with `reservedUntilTokenId`.
+
+Because `baseTokenId` is not initialized with `reservedUntilTokenId`, it's likely that all positions in the `tokenRecipient` will not be cleared as wrong token position will be assumed to belong to owner.
+
+Example(the most simple):
+- reservedUntilTokenId = 10, and we have 1 owner with 1% ownership, so `tokenRecipient[10] = owner1`
+- now `updateFounders` is called and because `baseTokenId` is initialized as 0, then `tokenRecipient[10]` was not cleared
+- same owner was set with percentage 1%(same percentage, so it's easier to explain), then `tokenRecipient[11] = owner1` and `tokenRecipient[10] = owner1` still remains
+
+Now `owner1` has 2 tokens out of 100 instead of 1.
+The problem is that in case if user has 1% then he receives only 1 additional token, but in case if he had bigger percentage, then he will get bigger amount of additional tokens.
+
+This bug can be triggered in case if dao uses `reservedUntilTokenId` or have migrated from l1 to l2(as it's likely that `reservedUntilTokenId` will not be 0 only after migration) and after that `updateFounders` was called.
+## Impact
+Owner receives additional vesting tokens.
+## Code Snippet
+Provided above
+## Tool used
+
+Manual Review
+
+## Recommendation
+Initialize `baseTokenId` with `reservedUntilTokenId` value.
diff --git a/001/005.md b/001/005.md
@@ -0,0 +1,181 @@
+Cold Blonde Sparrow
+
+high
+
+# The `Token::updateFounders()` func does not remove the previous founders, which leads to them being able to claim tokens from the DAO
+
+## Summary
+
+The `Token::updateFounders()` function does not remove the previous founders, which leads to them being able to claim tokens.
+## Vulnerability Detail
+
+Founders are added to the DAO's founders list during the deployment of the DAO by using the [Token::_addFounders()](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L92) function. As a result, the founder can obtain a token from the DAO. The `Token::_addFounders()` func starts from the [reservedUntilTokenId](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L161) number in order to assign it to the corresponding `tokenRecipient` array in the [code line 169](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L169):
+
+```solidity
+File: Token.sol
+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:             }
+```
+
+In the other hand, the owner can update the founders list using the [Token::updateFounders()](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L375) func. The function will remove the previous assigned founders in the [code line 420](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L420):
+
+```solidity
+File: Token.sol
+411:                 // Used to reverse engineer the indices the founder has reserved tokens in.
+412:                 uint256 baseTokenId;
+413: 
+414:                 for (uint256 j; j < cachedFounder.ownershipPct; ++j) {
+415:                     // Get the next index that hasn't already been cleared
+416:                     while (clearedTokenIds[baseTokenId] != false) {
+417:                         baseTokenId = (++baseTokenId) % 100;
+418:                     }
+419: 
+420:                     delete tokenRecipient[baseTokenId];
+421:                     clearedTokenIds[baseTokenId] = true;
+422: 
+423:                     emit MintUnscheduled(baseTokenId, i, cachedFounder);
+424: 
+425:                     // Update the base token id
+426:                     baseTokenId = (baseTokenId + schedule) % 100;
+427:                 }
+428:             }
+```
+
+A problem arises because the function starts from the [baseTokenId zero](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L412), an action that is incorrect because it does not consider the `reservedUntilTokenId` in the [code line 412](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L412). As a result, previous founders remain on the list, enabling them to claim a token from the DAO.
+
+## Impact
+
+The [Token::updateFounders()](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L375C14-L375C28) does not work as intended, it attempts to remove the previous founders from the `tokenRecipient[]` array but fails, leaving previous founders able to claim a token from the DAO.
+
+I created a test where the DAO is deployed using a list of two founders and a `reservedUntilTokenId=10`, then the owner updates the founders using another list of founders but the previous founders still are in the `tokenRecipient[]` array.
+
+```solidity
+// File: Token.t.sol
+// $ forge test --match-test "test_UpdateFundersDoesNotUseTheReservedUntilTokenId" -vvv
+//
+    function test_UpdateFundersDoesNotUseTheReservedUntilTokenId() public {
+        //
+        // The previous founders are not removed once the owner calls updateFounders() using another founders list causing that
+        // the previous founders are able to claim a token
+        createUsers(2, 1 ether);
+        address[] memory wallets = new address[](2);
+        uint256[] memory percents = new uint256[](2);
+        uint256[] memory vestExpirys = new uint256[](2);
+        unchecked {
+            for (uint256 i; i < 2; ++i) {
+                wallets[i] = otherUsers[i];
+                percents[i] = 1;
+                vestExpirys[i] = 4 weeks;
+            }
+        }
+        deployWithCustomFoundersAndCustomReserve(wallets, percents, vestExpirys, 10);
+        //
+        // assert totalFounders is 1
+        assertEq(token.totalFounders(), 2);
+        assertEq(token.totalFounderOwnership(), 2);
+        //
+        // The scheduledRecipient starts in number `10` because the token was deployed using reservedUntilTokenId = 10
+        assertEq(token.getScheduledRecipient(10).wallet, otherUsers[0]);
+        assertEq(token.getScheduledRecipient(11).wallet, otherUsers[1]);
+        //
+        // The founders are updated but the previous founders are not removed
+        IManager.FounderParams[] memory newFoundersArr = new IManager.FounderParams[](1);
+        newFoundersArr[0] = IManager.FounderParams({
+            wallet: address(0x06B59d0b6AdCc6A5Dc63553782750dc0b41266a3),
+            ownershipPct: 10,
+            vestExpiry: 2556057600
+        });
+        vm.prank(address(wallets[0]));
+        token.updateFounders(newFoundersArr);
+        //
+        // Assert the previous founder still has the baseTokenId 10 and 11 which is incorrect
+        // because the previous founders list should had been removed
+        assertEq(token.getScheduledRecipient(10).wallet, otherUsers[0]);
+        assertEq(token.getScheduledRecipient(11).wallet, otherUsers[1]);
+        assertEq(token.getScheduledRecipient(12).wallet, address(0x06B59d0b6AdCc6A5Dc63553782750dc0b41266a3));
+    }
+```
+
+Additionally, the next funcion is added to the `NounsBuilderTest.sol` file:
+
+```diff
++++ b/nouns-protocol/test/utils/NounsBuilderTest.sol
+@@ -274,6 +274,25 @@ contract NounsBuilderTest is Test {
+         setMockMetadata();
+     }
+
++    function deployWithCustomFoundersAndCustomReserve(
++        address[] memory _wallets,
++        uint256[] memory _percents,
++        uint256[] memory _vestExpirys,
++        uint256 _reservedUntilTokenId
++    ) internal virtual {
++        setFounderParams(_wallets, _percents, _vestExpirys);
++
++        setMockTokenParamsWithReserve(_reservedUntilTokenId);
++
++        setMockAuctionParams();
++
++        setMockGovParams();
++
++        deploy(foundersArr, tokenParams, auctionParams, govParams);
++
++        setMockMetadata();
++    }
++
+```
+
+## Code Snippet
+
+- [Token::_addFounders()](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L92)
+- [Token::updateFounders()](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L375)
+
+
+## Tool used
+
+Manual review
+
+## Recommendation
+
+Please, ensure the usage of `reservedUntilTokenId` in the [Token::updateFounders()](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L412) function:
+
+```diff
+File: Token.sol
+410: 
+411:                 // Used to reverse engineer the indices the founder has reserved tokens in.
+--                   uint256 baseTokenId;
+++                   uint256 baseTokenId = reservedUntilTokenId;
+413: 
+414:                 for (uint256 j; j < cachedFounder.ownershipPct; ++j) {
+415:                     // Get the next index that hasn't already been cleared
+416:                     while (clearedTokenIds[baseTokenId] != false) {
+417:                         baseTokenId = (++baseTokenId) % 100;
+418:                     }
+419: 
+420:                     delete tokenRecipient[baseTokenId];
+421:                     clearedTokenIds[baseTokenId] = true;
+422: 
+423:                     emit MintUnscheduled(baseTokenId, i, cachedFounder);
+424: 
+425:                     // Update the base token id
+426:                     baseTokenId = (baseTokenId + schedule) % 100;
+427:                 }
+428:             }
+```
+
+Moreover, adapt the necessary procedures if the `reservedUntilTokenId` changes via the [Token::setReservedUntilTokenId()](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L486C14-L486C37) function.
diff --git a/001/008.md b/001/008.md
@@ -0,0 +1,50 @@
+Uneven Clay Rook
+
+medium
+
+# reservedUntilTokenId should not be used to calculate vested positions
+
+## Summary
+Because `reservedUntilTokenId` is used to calculated vested positions for owners it's likely that founders will get bigger amount of tokens than they should after migration.
+## Vulnerability Detail
+`reservedUntilTokenId` is parameter that marks amount of tokens, that should not be used using auction. They should be minted [only by special minter](https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L211-L217). The reason we need this param is to make migration to l2. When migration will start, then `reservedUntilTokenId` will be set to the amount of already minted tokens. So later minter can mint them all to the recipients on l2. So when dao is deployed on l1 it will have `reservedUntilTokenId` as 0.
+
+Now we need to talk about how vesting positions are calculated for the owners.
+https://github.com/sherlock-audit/2023-09-nounsbuilder/blob/main/nouns-protocol/src/token/Token.sol#L161-L176
+```solidity
+                uint256 baseTokenId = reservedUntilTokenId;
+
+                // For each token to vest:
+                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 = (baseTokenId + schedule) % 100;
+                }
+            }
+```
+As you can see, `baseTokenId` is initialized with `reservedUntilTokenId`. On l1 it will be 0 likely. Then we go through the percentage of founder and mark his vesting positions.
+
+For example we have 2 founders with 51% and 5%.
+- Then founder1 will have `tokenRecipient[0 - 50]` as his 51 vesting positions and founder2 will have `tokenRecipient[51;71;91;52;72]` as vesting positions.
+- Then we minted 8 tokens through the action, which means that tokens[0-59] were minted and `founder1` got 51 token and `founder2` got 1 vesting token.
+- Then dao decided to move to l2 and `reservedUntilTokenId` was set to 60 as such amount of tokens were minted already.
+- Then on l2 during initialization vesting positions are recalculated using `reservedUntilTokenId`, so founder1 will have `tokenRecipient[0-10;60 - 99]` as his 51 vesting positions and founder2 will have `tokenRecipient[11;31;51;12;32]` as vesting positions.
+- So additional 8 tokens were minted using auction, which means that tokens[60-119] were minted and `founder1` again got 51 token and `founder2` got 1 vesting token.
+- As result we see that during that period founder1 got 102 vesting tokens, while founder2 got 2. And also we see that total minted amount is 120 and 104 of them is for founders, which is bigger than it should be.
+## Impact
+Incorrect vesting amount can occur after migration
+## Code Snippet
+Provided above
+## Tool used
+
+Manual Review
+
+## Recommendation
+I believe that you don't need to use `reservedUntilTokenId` to calculate vesting positions. Just start with 0 and it will work in all cases. Using `reservedUntilTokenId` you are protected that those amount ids will not be reused anymore and that's enough.