Nouns Builder contest - zzzitron's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L179

Vulnerability details

Impact

The Token as well as Auction cannot be used if the sum of ownershipPct is 100

Proof of Concept

    function test_poc_mintforever() 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 = 50;
        uint256 end = 4 weeks;

        unchecked {
            for (uint256 i; i < 2; ++i) {
                wallets[i] = otherUsers[i];
                percents[i] = pct;
                vestExpirys[i] = end;
            }
        }

        deployWithCustomFounders(wallets, percents, vestExpirys);

        assertEq(token.totalFounders(), 2);
        assertEq(token.totalFounderOwnership(), 100);

        Founder memory founder;

        unchecked {
            for (uint256 i; i < 100; ++i) {
                founder = token.getScheduledRecipient(i);

                if (i % 2 == 0) assertEq(founder.wallet, otherUsers[0]);
                else assertEq(founder.wallet, otherUsers[1]);
            }
        }

// // commented out as it will not stop
//         vm.prank(otherUsers[0]);
//         auction.unpause();

    }

In the proof of concept, there are two founders and they both share 50% of ownership. If the Auction should be unpaused, and therefore triggers to mint tokens, it will go into the infinite loop and eventually revert for out of gas.

// Token.sol

143     function mint() external nonReentrant returns (uint256 tokenId) {
144         // Cache the auction address
145         address minter = settings.auction;
146
147         // Ensure the caller is the auction
148         if (msg.sender != minter) revert ONLY_AUCTION();
149
150         // Cannot realistically overflow
151         unchecked {
152             do {
153                 // Get the next token to mint
154                 tokenId = settings.totalSupply++;
155
156                 // Lookup whether the token is for a founder, and mint accordingly if so
157             } while (_isForFounder(tokenId));
158         }
159
160         // Mint the next available token to the auction house for bidding
161         _mint(minter, tokenId);
162     }

177     function _isForFounder(uint256 _tokenId) private returns (bool) {
178         // Get the base token id
179         uint256 baseTokenId = _tokenId % 100;
180
181         // If there is no scheduled recipient:
182         if (tokenRecipient[baseTokenId].wallet == address(0)) {
183             return false;
184
185             // Else if the founder is still vesting:
186         } else if (block.timestamp < tokenRecipient[baseTokenId].vestExpiry) {
187             // Mint the token to the founder
188             _mint(tokenRecipient[baseTokenId].wallet, _tokenId);
189
190             return true;
191
192             // Else the founder has finished vesting:
193         } else {
194             // Remove them from future lookups
195             delete tokenRecipient[baseTokenId];
196
197             return false;
198         }
199     }

In the Token::mint, there is a while loop which will keep looping as long as _isForFounder returns true. The _isForFounder function will return true is the given _tokenId's recipient is still vesting. However, to check the recipient it is checking the baseTokenId which is _tokenId % 100 (in line 179 above snippet). Which means, if the tokenRecipient of 0 to 99 are currently vesting, it will keep returning true and the while loop in the mint function will not stop. The tokenRecipient was set in the _addFounders and if the sum of all founders' ownership percent is 100, the tokenRecipient will be filled up to 100.

Tools Used

None

Recommended Mitigation Steps

use _tokenId instead of baseTokenId.

<!-- zzzitron H01 -->

        uint256 baseTokenId = _tokenId % 100;

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/token/Token.sol#L118

Vulnerability details

Impact

It calculates wrong baseTokenId

Proof of Concept

In the _isForFounder checks the tokenRecipient only within [0-99] range, so presumable, the baseTokenId in the _addFounders were supposedly to wrap around 100. However, the modulo operator in the line 118 does not do that. The line 118 with modulo will give same result without modulo operator, so it is ineffective. Moreover, a new baseTokenId will be assigned by _getNextTokenId (line 110), which will just increase the tokenId.

// Token::_addFounders
110                    baseTokenId = _getNextTokenId(baseTokenId);
//...
118                    (baseTokenId += schedule) % 100;

// Token::_isForFounder
179        uint256 baseTokenId = _tokenId % 100;
//...
188             _mint(tokenRecipient[baseTokenId].wallet, _tokenId);

Tools Used

None

Recommended Mitigation Steps

It is not clear what was the intention of the line 118, but presumably baseTokenId = ( baseTokenId + schedule ) % 100. But since the baseTokenId is updated in the line 120 by the _getNextTokenId, the baseTokenId is not bound to the 100.

<!-- zzzitron M04 -->

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L580-L592

Vulnerability details

Impact

No more voting is possible if proposalThresholdBps and quorumThresholdBps is set to be larger than 10000

Proof of Concept

// Governor::initialize
 79         settings.proposalThresholdBps = SafeCast.toUint16(_proposalThresholdBps);
 80         settings.quorumThresholdBps = SafeCast.toUint16(_quorumThresholdBps);

// Governor
// proposalThreshold
466     function proposalThreshold() public view returns (uint256) {
467         unchecked {
468             return (settings.token.totalSupply() * settings.proposalThresholdBps) / 10_000;
469         }
470     }

// Governor
// quorum
473     function quorum() public view returns (uint256) {
474         unchecked {
475             return (settings.token.totalSupply() * settings.quorumThresholdBps) / 10_000;
476         }
477     }

// Governor
// setters for proposalThresholdBps and quorumThresholdBps
580     function updateProposalThresholdBps(uint256 _newProposalThresholdBps) external onlyOwner {
581         emit ProposalThresholdBpsUpdated(settings.proposalThresholdBps, _newProposalThresholdBps);
582
583         settings.proposalThresholdBps = SafeCast.toUint16(_newProposalThresholdBps);
584     }

588     function updateQuorumThresholdBps(uint256 _newQuorumVotesBps) external onlyOwner {
589         emit QuorumVotesBpsUpdated(settings.quorumThresholdBps, _newQuorumVotesBps);
590
591         settings.quorumThresholdBps = SafeCast.toUint16(_newQuorumVotesBps);
592     }

If the proposalThresholdBps is larger than 10000, even if a person with 100% of share cannot propose. If quorumThresholdBps is larger than 10000, even if 100% of voters vote for it, the propose will not pass. It means once a value of more than 10000 is set in either of proposalThresholdBps or quorumThresholdBps, no proposal can be proposed, or no proposal can have enough vote for making any more change. To update the settings, it should pass vote and executed through the treasury, however there can be no more passing votes, no more update in the settings is possible. Therefore, it is stuck in these values and can have no more votes. Any weth or eth in the treasury will also be inaccessible.

Currently, the upper limit for these variables are the max of uint16, which is 65535 or approximately 600%.

Tools Used

None

Recommended Mitigation Steps

Should set some reasonable upper limit for proposalThresholdBps and quorumThresholdBps.

<!-- zzzitron M03 -->

Lines of code

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

Vulnerability details

Impact

Founders can take a lot of tokens more than they are supposed to.

Proof of Concept

Below is a snippet of proof of concept for a founder takes a lot of tokens. The full test code can be found here. It was modified from Token.t.sol.

    function test_poc_FounderPctOverflow() 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 = 50;
        uint256 end = 4 weeks;

        unchecked {
            for (uint256 i; i < 2; ++i) {
                wallets[i] = otherUsers[i];
                percents[i] = pct;
                vestExpirys[i] = end;
            }
        }
        // Overflow was not checked upon cast
        percents[0] = uint256(type(uint8).max) + 51;

        deployWithCustomFounders(wallets, percents, vestExpirys);

        assertEq(token.totalFounders(), 2);
        assertEq(token.totalFounderOwnership(), 100);

        Founder memory founder;

        unchecked {
            for (uint256 i; i < 500; ++i) {
                founder = token.getScheduledRecipient(i);

                assertEq(founder.wallet, otherUsers[0]);
                // if (i % 2 == 0) assertEq(founder.wallet, otherUsers[0]);
                // else assertEq(founder.wallet, otherUsers[1]);
            }
        }
    }

In the Token::_addFounders the founders' ownershipPct was casted without checking for overflow:

// Token::_addFounders

// In the line 88 and 99, the `founderPct` was casted into uint8.
 87                 // Update the total ownership and ensure it's valid
 88                 if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

 99                 newFounder.ownershipPct = uint8(founderPct);

// In the line 102, the `founderPct` was used without casting
101                 // Compute the vesting schedule
102                 uint256 schedule = 100 / founderPct;

// In the line 108, the `founderPct` was used without casting
107                 // For each token to vest:
108                 for (uint256 j; j < founderPct; ++j) {
109                     // Get the available token id
110                     baseTokenId = _getNextTokenId(baseTokenId);
111
112                     // Store the founder as the recipient
113                     tokenRecipient[baseTokenId] = newFounder;
114
115                     emit MintScheduled(baseTokenId, founderId, newFounder);
116
117                     // Update the base token id
118                     (baseTokenId += schedule) % 100;
119                 }

For the totalOwnership and ownershipPct, the founderPct was casted into uint8. If the given founderPct is larger than type(uint8).max the founderPct will wrap around and it will pass the check for totalOwnership being less or equal to 100 (line 88 in the above code snippet). However, the founderPct was used in the schedule and in the loop without casting into uint8 (line 102 and 108 in the above snippet). Therefore, the founder can get a lot more tokens then they are supposed to get. Besides, the schedule will also set to be zero, in the case founderPct would be larger than zero.

In the above example proof of concept, the founder[0] will be given 50% of ownership, but they will get type(uint8).max + 51 tokens.

Tools Used

None

Recommended Mitigation Steps

The best solution would be make the FounderParams.ownershipPct as uint8 value. Easier solution would be add a line to check the ownershipPct can be casted safely. For example:

// Token::__addFounders
 87                 // Update the total ownership and ensure it's valid
+        	    require(founderPct <= type(uint8).max, "SafeCast: value doesn't fit in 8 bits");
 88                 if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP();

<!-- zzzitron H00 -->

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/auction/Auction.sol#L123

Vulnerability details

Impact

A bidder can outbid previous bid with the same value, if the (previous bid * minBidIncrement < 100).

Proof of Concept

// Auction.sol

// createBid
117             unchecked {
118                 // Compute the minimum bid
119                 minBid = highestBid + ((highestBid * settings.minBidIncrement) / 100);
120             }
121
122             // Ensure the incoming bid meets the minimum
123             if (msg.value < minBid) revert MINIMUM_BID_NOT_MET();

331     function setMinimumBidIncrement(uint256 _percentage) external onlyOwner {
332         settings.minBidIncrement = SafeCast.toUint8(_percentage);
333
334         emit MinBidIncrementPercentageUpdated(_percentage);
335     }

When the minBid (defined in the line 119) is the same as the current highestBid, one can call createBid with the same value as the current highestBid (line 123). It means that the new bidder will get the Bid, even though the previous bidder has the same bid and was earlier. The minBid can be the same as the highesBid when highestBid * minBidIncrement is less than 100. So either the highestBid or minBinIncrement is too small, a bidder can overbid the precious one with the same amount of value.

The first bid should be higher or equal to the reservePrice. However, there is no safe guard against setting small reservePrice and minBidIncrement.

For example, let's say the settings.minBidIncrement is set to zero. Alice called createBid with 1 ether and is the current highestBidder with the highestBid of 1 ether. Bob calls createBid with 1 ether. The minBid in the line 119 will be 1ether as minBidIncrement is set to zero. In the line 123 the msg.value is 1 ether is the same as minBid therefore it will not revert. And now Bob is the highestBidder even though he bid the same value after Alice.

Tools Used

None

Recommended Mitigation Steps

Revert if the msg.value is the same as the minBid:

// Auction.sol

// createBid
117             unchecked {
118                 // Compute the minimum bid
119                 minBid = highestBid + ((highestBid * settings.minBidIncrement) / 100);
120             }
121
122             // Ensure the incoming bid meets the minimum
- 		if (msg.value < minBid) revert MINIMUM_BID_NOT_MET();
+		if (msg.value <= minBid) revert MINIMUM_BID_NOT_MET();

<!-- zzzitron M00 -->