Nouns Builder contest - pcarranzav's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L179-L189

Vulnerability details

Impact

Delegation should transfer an account's votes to the other account. Instead, the way the code is written in _delegate(), the votes are only transferred if there is a previous delegation; otherwise, the prevDelegate variable is zero, and _moveDelegateVotes behaves as when a new token is minted (_from parameter is zero).

The result is that an attacker holding two tokens in separate accounts can delegate from one account to the other and viceversa, and get four votes instead of two.

Proof of Concept

The following failing test illustrates the issue: founder delegates to founder2, so they should not have any votes anymore, and founder2 should get two votes. Instead, founder still has 1 vote, and founder2 has 2.

When founder2 then delegates to founder, they should again each have 1 vote (delegated from the other founder). Instead, they each have 2 votes, doubling their voting power.

diff --git a/test/Token.t.sol b/test/Token.t.sol
index 08eadd1..4b9098e 100644
--- a/test/Token.t.sol
+++ b/test/Token.t.sol
@@ -85,6 +85,44 @@ contract TokenTest is NounsBuilderTest, TokenTypesV1 {
         assertEq(token.getVotes(address(auction)), 1);
     }
 
+    function test_DelegateDoubleVote() public {
+        deployMock();
+
+        vm.prank(founder);
+        auction.unpause();
+
+        assertEq(token.totalSupply(), 3);
+
+        assertEq(token.ownerOf(0), founder);
+        assertEq(token.ownerOf(1), founder2);
+        assertEq(token.ownerOf(2), address(auction));
+
+        assertEq(token.balanceOf(founder), 1);
+        assertEq(token.balanceOf(founder2), 1);
+        assertEq(token.balanceOf(address(auction)), 1);
+
+        assertEq(token.getVotes(founder), 1);
+        assertEq(token.getVotes(founder2), 1);
+        assertEq(token.getVotes(address(auction)), 1);
+
+        // Now founder delegates to founder2
+        vm.prank(founder);
+        token.delegate(founder2);
+
+        // So founder's vote should now be in founder2's hands
+        assertEq(token.getVotes(founder), 0);
+        assertEq(token.getVotes(founder2), 2);
+
+        // founder2 delegates their own vote -
+        // so now each should get 1 vote again
+        vm.prank(founder2);
+        token.delegate(founder);
+
+        assertEq(token.getVotes(founder), 1);
+        assertEq(token.getVotes(founder2), 1);
+
+    }
+
     function test_MaxOwnership100Founders() public {
         createUsers(100, 1 ether);
 

Tools Used

VSCode, foundry

Recommended Mitigation Steps

When there is no previous delegation, transfer the user's own votes instead of creating new ones:

diff --git a/src/lib/token/ERC721Votes.sol b/src/lib/token/ERC721Votes.sol
index 3e64720..b9e5dd7 100644
--- a/src/lib/token/ERC721Votes.sol
+++ b/src/lib/token/ERC721Votes.sol
@@ -185,6 +185,9 @@ abstract contract ERC721Votes is IERC721Votes, EIP712, ERC721 {
 
         emit DelegateChanged(_from, prevDelegate, _to);
 
+        if (prevDelegate == address(0)) {
+            prevDelegate = _from;
+        }
         // Transfer voting weight from the previous delegate to the new delegate
         _moveDelegateVotes(prevDelegate, _to, balanceOf(_from));
     }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L102-L120

Vulnerability details

Impact

The way the vesting schedule is implemented doesn't validate that the baseTokenId is between 0 and 99.

But only baseTokenIds between 0 and 99 will actually produce tokens that are assigned to a founder.

Therefore, depending on the combination of percentages, while looping over the token IDs, some of the percentage points for a founder can be assigned to a baseTokenId that will never be redeemed.

Moreover, when incrementing the tokenId in line 118, the modulo 100 operation isn't stored back into the baseTokenId variable, so the addition doesn't actually wrap around.

Founders will therefore lose and never vest part of their ownership percentage.

Proof of Concept

The following diff shows a test that fails because of this issue. Two founders get assigned 85 and 7 percent of tokens, respectively. The first founder gets 85 percent of tokens, but the second one ends up getting only 2 percent:

diff --git a/test/Token.t.sol b/test/Token.t.sol
index 08eadd1..0bc28f9 100644
--- a/test/Token.t.sol
+++ b/test/Token.t.sol
@@ -188,6 +188,52 @@ contract TokenTest is NounsBuilderTest, TokenTypesV1 {
         }
     }
 
+    function test_UnevenOwnership2Founders() public {
+        createUsers(2, 1 ether);
+
+        address[] memory wallets = new address[](2);
+        uint256[] memory percents = new uint256[](2);
+        uint256[] memory vestExpirys = new uint256[](2);
+
+        uint256 founder0Pct = 85;
+        uint256 founder1Pct = 7;
+        uint256 end = 4 weeks;
+
+        wallets[0] = otherUsers[0];
+        percents[0] = founder0Pct;
+        vestExpirys[0] = end;
+
+        wallets[1] = otherUsers[1];
+        percents[1] = founder1Pct;
+        vestExpirys[1] = end;
+
+        deployWithCustomFounders(wallets, percents, vestExpirys);
+
+        assertEq(token.totalFounders(), 2);
+        assertEq(token.totalFounderOwnership(), founder0Pct + founder1Pct);
+
+        Founder memory founder;
+
+        uint256 founder0Tokens = 0;
+        uint256 founder1Tokens = 0;
+        uint256 unnassignedTokens = 0;
+        unchecked {
+            for (uint256 i; i < 1000; ++i) {
+                founder = token.getScheduledRecipient(i);
+                if (founder.wallet == otherUsers[0]) {
+                    founder0Tokens += 1;
+                } else if (founder.wallet == otherUsers[1]) {
+                    founder1Tokens += 1;
+                } else {
+                    unnassignedTokens += 1;
+                }
+            }
+        }
+        assertEq(founder0Tokens, founder0Pct * 10);
+        assertEq(founder1Tokens, founder1Pct * 10);
+        assertEq(unnassignedTokens, (100 - founder0Pct - founder1Pct)*10);
+    }
+
     function testRevert_OnlyAuctionCanMint() public {
         deployMock();

Tools Used

VSCode, foundry

Recommended Mitigation Steps

Validate that the baseTokenId is < 100, and if not, start counting from 0 again. Also, assign the result of the modulo 100 to the baseTokenId variable. This fixes the test shown above:

diff --git a/src/token/Token.sol b/src/token/Token.sol
index afad142..dc3f814 100644
--- a/src/token/Token.sol
+++ b/src/token/Token.sol
@@ -108,14 +108,16 @@ contract Token is IToken, UUPS, ReentrancyGuard, ERC721Votes, TokenStorageV1 {
                 for (uint256 j; j < founderPct; ++j) {
                     // Get the available token id
                     baseTokenId = _getNextTokenId(baseTokenId);
-
+                    if (baseTokenId >= 100) {
+                        baseTokenId = _getNextTokenId(0);
+                    }
                     // Store the founder as the recipient
                     tokenRecipient[baseTokenId] = newFounder;
 
                     emit MintScheduled(baseTokenId, founderId, newFounder);
 
                     // Update the base token id
-                    (baseTokenId += schedule) % 100;
+                    baseTokenId = (baseTokenId + schedule) % 100;
                 }
             }

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L78-L125

Vulnerability details

Impact

founderPct is a uint256, but the validation in line 88 casts it to uint8 before checking if the addition goes over the maximum. Later, in line 102, the uint256 number is used to set the vesting schedule. Moreover, the uint256 value is used in the loop to define the mint schedule in line 108

This means a founder can use any percentage such that founderPct % 256 <= 100 and the initialization will succeed, producing an invalid number of tokens assigned to the founder.

Proof of Concept

Adding this failing test shows the problem (note we're using a founder percentage of 512!):

diff --git a/test/Token.t.sol b/test/Token.t.sol
index 08eadd1..89a0234 100644
--- a/test/Token.t.sol
+++ b/test/Token.t.sol
@@ -198,6 +198,28 @@ contract TokenTest is NounsBuilderTest, TokenTypesV1 {
         token.mint();
     }
 
+    function test_InvalidOwnershipPct() public {
+        createUsers(2, 1 ether);
+
+        address[] memory wallets = new address[](2);
+        uint256[] memory percents = new uint256[](2);
+        uint256[] memory vestExpirys = new uint256[](2);
+
+        uint256 pct = 512;
+        uint256 end = 4 weeks;
+
+        unchecked {
+            for (uint256 i; i < 2; ++i) {
+                wallets[i] = otherUsers[i];
+                percents[i] = pct;
+                vestExpirys[i] = end;
+            }
+        }
+
+        vm.expectRevert(abi.encodeWithSignature("INVALID_FOUNDER_OWNERSHIP()"));
+        deployWithCustomFounders(wallets, percents, vestExpirys);
+    }
+
     function testRevert_OnlyAuctionCanBurn() public {
         deployMock();

Tools Used

VSCode, foundry

Recommended Mitigation Steps

Validate founderPct before casting it to uint8:

diff --git a/src/token/Token.sol b/src/token/Token.sol
index afad142..6cad99d 100644
--- a/src/token/Token.sol
+++ b/src/token/Token.sol
@@ -84,6 +84,8 @@ contract Token is IToken, UUPS, ReentrancyGuard, ERC721Votes, TokenStorageV1 {
                 // Continue if no ownership is specified
                 if (founderPct == 0) continue;
 
+                // Ensure ownership percentage is within bounds
+                if (founderPct > 100) revert INVALID_FOUNDER_OWNERSHIP();
                 // Update the total ownership and ensure it's valid
                 if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();
 

Summary

I've separately reported what I believe are a high severity issue and a medium severity issue, both related to the token allocation for founders. That's the section of the code that I've (so far) been able to spend more time on, and I believe that particular section could use a refactor to make errors like these less likely.

The rest of the code looks okay to me so far. I'm including in this QA report one low-severity bug, and a few non-critical suggestions. The first of these (Immutability of implementations) is an architectural suggestion that I think could make the code more future-proof. The rest are simply style suggestions.

Other than that, even though I could find no vulnerabilities in these, I would recommend extra care and testing in:

• the proposal ID generation and hashing (https://github.com/code-423n4/2022-09-nouns-builder/blob/main/Governor.sol#L331 and https://github.com/code-423n4/2022-09-nouns-builder/blob/main/Governor.sol#L98) - I couldn't find a way to produce a collision, but an attacker somehow producing a collision in the ABI-encoded parameters would be able to void a proposal or call the wrong contract/function
• the upgrade path for Governor and Treasury implementations; the fact that these contracts are both owned by the Treasury make the whole thing a bit complex, which makes it look risky as well. Having some tests for Treasury upgrading Governor, and Treasury upgrading itself, would be beneficial.

Low severity

Blockhash of current block is zero and therefore not random

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/metadata/MetadataRenderer.sol#L251

In _generateSeed() I assume blockhash is used to introduce randomness, but blockhash(block.number) will always be zero. I recommend using blockhash(block.number - 1).

Non-critical

Immutability of implementations

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L25-L37

The pattern of using immutable variables in Manager is clever, but it doesn’t allow updating the default implementation for the contracts. This means any vulnerability found in the contracts after deployment would always be present in a newly-created DAO, until the founder upgrades to the newer implementation.

I understand it’d be a big change to make the base implementations mutable (especially because you then have to somehow figure out what creation hash to use to get the addresses in getAddresses).

But there is an easier alternative: providing a way to atomically deploy and upgrade a DAO to the latest version of each implementation. This could be done by keeping the latest version of each type of contract in Manager storage; the deploy() function can then check if there is a newer version for each contract and upgrade to it before returning.

Named returns

e.g. in https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/manager/Manager.sol#L105-L109

I recommend not using named returns (https://github.com/ethereum/solidity/issues/1401) and instead specifying the meaning of the return values in the NatSpec documentation.

Auction contract and Auction struct having the same name is confusing

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L22 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L92

Recommendation: change the name of one of the two, e.g. rename the contract to AuctionHouse

Assignment in conditional

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/managed/Managed.sol#L117

Having this assignment in conditional is confusion-prone, it would be safer to assign when declaring address founder.

Long blocks with unchecked math

e.g. https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L201-L234

While I can see the benefit of saving gas with unchecked math where it's guaranteed there'll be no overflows, the use of long blocks of code with unchecked makes it more likely that future code changes will introduce operations that can overflow.

I'd recommend applying unchecked{} to the specific statements where the lack of overflows is guaranteed.