Nouns Builder contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

In ERC721Votes, delegated voting power is neither transferred or cancelled when there is a transfer. An attacker could use this to generate an infinity of voting power.

Proof of Concept

This line is incorrect as it should transfer the voting power from the delegate if there is one.

Consider the following test:

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

import { Test, console } from "forge-std/Test.sol";
import { IERC721Votes, ERC721Votes } from "../src/lib/token/ERC721Votes.sol";

contract Token is ERC721Votes {
    constructor(address alice) {
        _mint(alice, 1);
    }
}

contract ERC721VotesTest is Test {
    address public alice = vm.addr(0xA);
    address public bob = vm.addr(0xB);
    address public charlie = vm.addr(0xC);

    Token public token;

    function setUp() public virtual {
        token = new Token(alice);
    }

    function test_transferOfDelegation() public {
        // Alice delegates to Charlie
        vm.prank(alice);
        token.delegate(charlie);
        // Therefore Charlie has a voting power of 1
        assertEq(token.getVotes(charlie), 1);

        // Alice sends the token to Bob
        vm.prank(alice);
        token.transferFrom(alice, bob, 1);
        // ISSUE: Charlie still have a voting power of 1 despite the transfer
        assertEq(token.getVotes(charlie), 1);
        assertEq(token.getVotes(bob), 1);
    }
}

Note that by repeating the operation anyone is able to generate an infinity of voting power.

Recommended Mitigation Steps

Check OpenZeppelin's implementation which handle this case correctly: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/6a8d977d2248cf1c115497fccfd7a2da3f86a58f/contracts/governance/utils/Votes.sol#L150

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#L189

Vulnerability details

Impact

By default delegation points to the zero address, so every one is self-delegating, which differs from OpenZeppelin contract. But voting power is not transferred from the owner when delegate is called, which therefore still holds voting power.

This leads to a situation where voting power is duplicated when there is a delegation !

Proof of Concept

The issue is that when delegating for the first time, _moveDelegateVotes is called with the from=address(0) which is not coherent with the self-delegation by default.

Consider the following test:

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

import { Test, console } from "forge-std/Test.sol";
import { IERC721Votes, ERC721Votes } from "../src/lib/token/ERC721Votes.sol";

contract Token is ERC721Votes {
    constructor(address alice) {
        _mint(alice, 1);
    }
}

contract ERC721VotesTest is Test {
    address public alice = vm.addr(0xA);
    address public charlie = vm.addr(0xC);

    Token public token;

    function setUp() public virtual {
        token = new Token(alice);
    }

    function test_delegates() public {
        // By default Alice has a voting power of 1 and charlie of 0.
        assertEq(token.getVotes(alice), 1);
        assertEq(token.getVotes(charlie), 0);

        // Alice delegates to charlie
        vm.prank(alice);
        token.delegate(charlie);

        // ISSUE: Alice still have a voting power of 1 despite her delegation
        assertEq(token.getVotes(alice), 1);
        assertEq(token.getVotes(charlie), 1);
    }
}

Recommended Mitigation Steps

Carefully handle the self-delegation by default. For instance, there is a new delegation, in _delegate, prevDelegate needs to be delegates(_from).

Lines of code

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

Vulnerability details

Impact

There could be tokens minted between the quorum computation and the vote, which would lead to a quorum lower than intended. It could be an issue at the beginning of the Token lifecycle, when the total supply is still low.

Proof of Concept

When creating a proposal in the Governor contract, proposal.quorumVotes is computed during the propose tx. However tokens could be minted after the propose in the same block. In this case, they'll still have a vote during the proposal due to how token.getPastVotes works, therefore the quorum will be lower than intended given the actual total supply.

Recommended Mitigation Steps

Either compute the total supply afterwards, for example at the beginning of the vote, either takes the vote with a timestamp of proposal.timeCreated - 1 to not count the block during which the tx was submitted.

[NC - 01] - Incorrect comment https://github.com/code-423n4/2022-09-nouns-builder/blob/7e9fddbbacdd7d7812e912a369cfd862ee67dc03/src/lib/token/ERC721Votes.sol#L204

"If this isn't a token mint" -> It could also be a first delegation