Nouns Builder contest - CertoraInc's results

Lines of code

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

Vulnerability details

Impact

In each iteration of the inner loop in the Token._addFounders function the base token id is incremented by schedule, which is 100 / founderPct, and token modulo 100. However, wrong placement of the parenthesis leads to the modulo 100 operation not being saved to the baseTokenId variable, and thus this variable's value can greater than 100.

(baseTokenId += schedule) % 100;

Proof of Concept

To verify this bug I created a foundry test. You can add it to the test folder and run it with forge test --match-test testWrongParenthesisBug.

This test deploys a token implementation and an ERC1967 proxy that points to it, and initializes the proxy using an array of 3 founders, having 25%, 25% and 50% ownership percents.

After the call to initialize, the test counts how many of token ids 0 to 99 (the base token ids) belong to each founder, and shows that the two first founders received 25 token ids but the third one didn't get 50 (if you'll add the -vv, you'll see that he also received 25 base token ids).

What actually happens here is that the first founder gets token ids 0, 4, ..., 96, the second founder gets 1, 5, ..., 97, and when its the third founders iteration, he gets token ids 2, 6, ..., 98, but then (because of the wrong parenthesis location) the baseTokenId is incremented to 102 instead of being zeroed and moving on to 3.

// The relative path of this file is "test/WrongParenthesis.t.sol"

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import { Test } from "forge-std/Test.sol";
import { console } from "forge-std/console.sol";

import { IManager } from "../src/manager/Manager.sol";
import { IToken, Token } from "../src/token/Token.sol";

import { TokenTypesV1 } from "../src/token/types/TokenTypesV1.sol";

import { ERC1967Proxy } from "../src/lib/proxy/ERC1967Proxy.sol";

contract WrongParenthesis is Test, TokenTypesV1 {

    Token imp;
    address proxy;
    
    function setUp() public virtual {
        // Deploying the implementation and the proxy
        imp = new Token(address(this));
        proxy = address(new ERC1967Proxy(address(imp), ""));
    }

    function testWrongParenthesisBug() public {

        IToken token = IToken(proxy);

        address founderA = address(0xA);
        address founderB = address(0xB);
        address founderC = address(0xC);

        // Creating 3 founders with 25%, 25%, 50% ownership
        IManager.FounderParams[] memory founders = new IManager.FounderParams[](3);
        founders[0] = IManager.FounderParams({
            wallet: founderA,
            ownershipPct: 25,
            vestExpiry: 1 weeks
        });
        founders[1] = IManager.FounderParams({
            wallet: founderB,
            ownershipPct: 25,
            vestExpiry: 1 weeks
        });
        founders[2] = IManager.FounderParams({
            wallet: founderC,
            ownershipPct: 50,
            vestExpiry: 1 weeks
        });

        // Initializing the proxy with the founders data
        token.initialize(   
            founders, 
            // we don't care about these
            abi.encode("", "", "", "", ""),
            address(0),
            address(0)
        );

        uint founderABaseTokensCount;
        uint founderBBaseTokensCount;
        uint founderCBaseTokensCount;
        // Counting how much base token ids each founder has (in a correct implementation this should be 25, 25, 50)
        for (uint i; i < 100; ++i) {
            address founder = token.getScheduledRecipient(i).wallet;
            if (founder == founderA) {
                founderABaseTokensCount++;
            } else if (founder == founderB) {
                founderBBaseTokensCount++;
            } else if (founder == founderC) {
                founderCBaseTokensCount++;
            }
        }

        // Showing what not all the base tokens are owned by the founders, and that not all the founders received their share
        assertEq(founderABaseTokensCount == 25, true);
        assertEq(founderBBaseTokensCount == 25, true);
        assertEq(founderCBaseTokensCount != 50, true);

        console.log(founderCBaseTokensCount); // 25 (instead of 50) - run with `-vv` to view this log

        // Run with `forge test --match-test testWrongParenthesisBug [-vv]`
        // Results:
        //      [PASS] testWrongParenthesisBug() (gas: 2989117)
        //      Logs:
        //          25
        // Great success
    }
}

Tools Used

Manual audit & foundry for PoC

Recommended Mitigation Steps

Fix this line to be:

baseTokenId = (baseTokenId + schedule) % 100;

After fixing the contract as I suggested, the test fails and the results are:

[FAIL. Reason: Assertion failed.] testWrongParenthesisBug() (gas: 2969396)
Logs:
  Error: a == b not satisfied [bool]
    Expected: true
      Actual: false
  50

We can see that the test fails and the printed value is 50 as expected.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L151

Vulnerability details

Impact

The Governor contract doesn't remove the proposal data from the storage after it was executed, which prevent the same proposal from being proposed again.

Proof of Concept

1. Alice proposes a proposal and it fails. The proposal id is 0xdeadbeef.
2. She wants to propose the same proposal again. She calls the propose function with the same parameters as the previous one.
3. The proposal id is calculated and is the same - 0xdeadbeef.
4. The propose function assures that the proposal doesn't exist, and because the previous (identical) proposal wasn't removed, this check will fail and the function will revert.

   // Get the pointer to store the proposal
   Proposal storage proposal = proposals[proposalId];
   
   // Ensure the proposal doesn't already exist
   if (proposal.voteStart != 0) revert PROPOSAL_EXISTS(proposalId);

Tools Used

Manual audit

Recommended Mitigation Steps

Add functionality to remove the proposal's data in case it is executed, canceled, vetoed, defeated or expired.

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/governance/governor/Governor.sol#L167-L168

Vulnerability details

Impact

The propose function saves parameters about the proposal, like proposal threshold and quorum. These parameters depends on the total supply in the current timestamp. However, the total supply can change after these values have been calculated and saved to storage, for example by calling the Auction.settleCurrentAndCreateNewAuction function in the same transaction. The new token will be able to vote in this proposal, because the votes are determined by the timestamp that the proposal was created at.

Proof of Concept

1. The attacker deploys a smart contract that will place bids in the auction and have the attack logic.
2. The attacker is about to win the auction, and the Auction.settleCurrentAndCreateNewAuction function can be called.
3. The attacker wants to pass a proposal, so he calls the attack logic, which proposes a proposal and then mints the new token by calling the Auction.settleCurrentAndCreateNewAuction function.

Assume the totalSupply was 100 before the attacker minted the new token, the proposal threshold is 1% and the quorum threshold is 20%. The attacker can propose a proposal which its proposal threshold will be 1 / 101 = 0.99%, and its quorum will be 20 / 101 = 19.801%, instead of 1% and 20%.

Tools Used

Manual audit

Recommended Mitigation Steps

Calculate the values based on the total supply at block.timestamp - 1, or calculate them after the current timestamp has passed.

Lines of code

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

Vulnerability details

Impact

The initialize function of the Token contract receives an array of FounderParams, which contains the ownership percent of each founder as a uint256. The initialize function checks that the sum of the percents is not more than 100, but the value that is added to the sum of the percent is truncated to fit in uint8. This leads to an error because the value that is used for assigning the base tokens is the original, not truncated, uint256 value.

This can lead to wrong assignment of the base tokens, and can also lead to a situation where not all the users will get the correct share of base tokens (if any).

Proof of Concept

To verify this bug I created a foundry test. You can add it to the test folder and run it with forge test --match-test testFounderGettingAllBaseTokensBug.

This test deploys a token implementation and an ERC1967 proxy that points to it, and initializes the proxy using an array of 2 founders, each having 256 ownership percent. The value which is added to the totalOwnership variable is a uint8, and when truncating 256 to fit in a uint8 it will turn to 0, so this check will pass.

After the call to initialize, the test asserts that all the base token ids belongs to the first founder, which means the second founder didn't get any base tokens at all.

What actually happens here is that the first founder gets the first 256 token ids, and the second founder gets the next 256 token ids, but because the base token is calculated % 100, only the first 100 matters and they will be owned by the first owner.

This happens because schedule, which is equal to 100 / founderPct, will be zero (100 / 256 == 0 due to uint div operation), and the base token id won't be updated in (baseTokenId += schedule) % 100 (this line contains another mistake, which will be reported in another finding). The place where it will be updated is in the _getNextTokenId, where it will be incremented by 1.

This exploit can work as long as the sum of the percents modulo 256 (truncation to uint8) is not more than 100.

// The relative path of this file is "test/FounderGettingAllBaseTokensBug.t.sol"

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import { Test } from "forge-std/Test.sol";

import { IManager } from "../src/manager/Manager.sol";
import { IToken, Token } from "../src/token/Token.sol";

import { TokenTypesV1 } from "../src/token/types/TokenTypesV1.sol";

import { ERC1967Proxy } from "../src/lib/proxy/ERC1967Proxy.sol";

contract FounderGettingAllBaseTokensBug is Test, TokenTypesV1 {

    Token imp;
    address proxy;
    
    function setUp() public virtual {
        // Deploying the implementation and the proxy
        imp = new Token(address(this));
        proxy = address(new ERC1967Proxy(address(imp), ""));
    }

    function testFounderGettingAllBaseTokensBug() public {

        IToken token = IToken(proxy);

        address chadFounder = address(0xdeadbeef);
        address betaFounder = address(0xBBBBBBBB); // beta

        // Creating 2 founders with `ownershipPct = 256`
        IManager.FounderParams[] memory founders = new IManager.FounderParams[](2);
        founders[0] = IManager.FounderParams({
            wallet: chadFounder,
            ownershipPct: 256,
            vestExpiry: 1 weeks
        });
        founders[1] = IManager.FounderParams({
            wallet: betaFounder,
            ownershipPct: 256,
            vestExpiry: 1 weeks
        });

        // Initializing the proxy with the founders data
        token.initialize(   
            founders, 
            // we don't care about these
            abi.encode("", "", "", "", ""),
            address(0),
            address(0)
        );

        // Asserting that the chad founder got all the base token ids
        // (`tokenId % 100` is calculated to get the base token, so it is enough to check only the first 100 token ids)
        for (uint i; i < 100; ++i) {
            assertEq(token.getScheduledRecipient(i).wallet == chadFounder, true);
        }

        // Run with `forge test --match-test testFounderGettingAllBaseTokensBug`
        // Results:
        //      [PASS] testFounderGettingAllBaseTokensBug() (gas: 13537465)
        // Great success
    }

Tools Used

Manual audit & foundry for the PoC

Recommended Mitigation Steps

Don't truncate the founderPct variable to a uint8 when adding it to the totalOwnership variable, or alternatively check that it is less than type(uint8).max (or less or equal to 100). After applying this fix and running the test again, the result is:

[FAIL. Reason: INVALID_FOUNDER_OWNERSHIP()] testFounderGettingAllBaseTokensBug() (gas: 58674)

QA Report

• The initialize function of the Treasury contract doesn't emit the GracePeriodUpdated event
• ETH can be locked in the Auction contract - either by transferring value in the constructor (which is payable) or by transferring it without calling the fallback/receive functions (for example using the self destruct operation)
• Pragmas should be locked to a specific compiler version, to avoid contracts getting deployed using a different version, which may have a greater risk of undiscovered bugs.

Gas Optimizations

• The _settleAuction function uses the storage variable auction instead of using the cached variable _auction

  if (auction.settled) revert AUCTION_SETTLED();
  

• Use the calldata modifier instead of the memory modifier on struct and array arguments to save gas. For example, the propose function receives multiple array arguments, which are passed as memory arguments and can be passed as calldata to save gas.

  function propose(
      address[] memory _targets,
      uint256[] memory _values,
      bytes[] memory _calldatas,
      string memory _description
  ) external returns (bytes32) {