Ex-token holders are still able to cast votes on proposals under certain circumstances

[code423n4]

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Votes.sol#L70-L76 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Votes.sol#L184-L200 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Votes.sol#L210-L264

Vulnerability details

When casting a vote, an address is limited to a certain amount of votes derived from ds.nouns.getPriorVotes. However, due to the nature of ds.nouns.getPriorVotes, the amount of votes available to an address solely depends on the amount of tokens they held when a proposal becomes of Active state. Consequently, if the token is transferred to another address during this Active state, then the new token holder will not be able to vote on the current proposal, as they were not included in the ds.nouns.getPriorVotes snapshot. However, if the original owner did not vote on the active proposal when they owned the token, they will still be able to vote on the proposal if it is still active, even though they are technically no longer a token holder.

Impact

The likelihood of this bug occurring is high, as tokens are traded on secondary marketplaces all the time, many of which will probably happen during active proposals. However, the impact of this bug is low, as the total number of votes remains unchanged throughout the proposal process, all of which result in a medium severity. The issue with this bug lies in the fact that addresses who are no longer token holders are still able to cast a valid vote, and being as how they are no longer members of the DAO, they could vote against the majority to make the succeeding of the proposal more difficult.

Proof of Concept

When calling the castVote function https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Votes.sol#L70-L76

an internal call is made to castVoteInternal https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Votes.sol#L184-L200

which checks to see if the vote is Active or in the Objection period. If it isn't, then the vote is reverted with 'NounsDAO::castVoteInternal: voting is closed'. However, if it is Active, then it makes another internal call to https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Votes.sol#L210-L264

The issue lies with this line: https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Votes.sol#L222

uint96 votes = ds.nouns.getPriorVotes(voter, proposalVoteSnapshotBlock(ds, proposalId, proposal));

If the call to this function makes it passed all of the checks, then the forVotes, againstVotes, or abstainVotes are increased by the amount of votes returned from the ds.nouns.getPriorVotes call. And because this is determined at the start of the proposal becoming Active, it is a lagging indicator of how many votes an address should actually have.

With what has been described above, the following test proves that a non-token holder can still cast votes on Active proposals under certain circumstances:

contract WoolCentaurGivenStateActive is NounsDAOLogicV3BaseTest {
    address proposer = makeAddr('proposer');
    address ogOwner = makeAddr('ogOwner');
    address voter = makeAddr('voter');
    uint256 proposalId;

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

        mintTo(proposer);
        mintTo(ogOwner);

        proposalId = propose(proposer, proposer, 0.01 ether, '', '', '');
    }

    function test_WoolCentaurs_givenStateActive() public {
        //The block of the proposal was created
        emit log_named_uint("block.number", block.number);

        vm.prank(ogOwner);
        vm.roll(block.number + dao.proposalUpdatablePeriodInBlocks() + dao.votingDelay() + 1);
        //To push the proposal into the active state to allow voting
        assertTrue(dao.state(proposalId) == NounsDAOStorageV3.ProposalState.Active);

        // Transferring the token to another address during the active state
        vm.prank(ogOwner);
        nounsToken.transferFrom(ogOwner, voter, 2);

        // Verifying that the token was indeed transferred
        emit log_named_uint("balanceOfOgOwner", nounsToken.balanceOf(ogOwner));
        emit log_named_uint("balanceOfVoter", nounsToken.balanceOf(voter));

        // This is just to prove that the ogOwner is able to cast a vote in any block within the active state
        vm.roll(block.number + 5000);
        emit log_named_uint("block.number", block.number);

        vm.prank(ogOwner);
        dao.castVote(proposalId, 1);
        // Verifying that the ogOwner's vote contributed to the .forVotes. Initially, the .forVotes is set to 0.
        emit log_named_uint("votes_first", dao.proposals(proposalId).forVotes);
        // Check to see if the new token owner, voter, is able to vote
        vm.prank(voter);
        dao.castVote(proposalId, 1);
        // Check to see if the voter's vote counted.  We will see that it didn't because the .forVotes was not incremented since the last check
        emit log_named_uint("votes_second", dao.proposals(proposalId).forVotes);

        // This is just to prove that the ogOwner's vote contributed to the proposal being succeeded
        vm.roll(block.number + dao.votingPeriod());
        assertTrue(dao.state(proposalId) == NounsDAOStorageV3.ProposalState.Succeeded);
    }
}

and here are the logs:

[PASS] test_WoolCentaurs_givenStateActive() (gas: 356257)
Logs:
  block.number: 3
  balanceOfOgOwner: 0
  balanceOfVoter: 1
  block.number: 5015
  votes_first: 1
  votes_second: 1

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 33.37ms

As you can see, the token that was originally minted by the ogOwner was transferred to the voter during the Active state. However, even after no longer being a token owner, ogOwner was still able to cast a vote and have it contribute to the succeeding of the proposal. Furthermore, the vote cast by the voter did not count.

Tools Used

Foundry

Recommended Mitigation Steps

Using the ds.nouns.balanceOf along with the ds.nouns.getPriorVotes will prevent ex-token holders from casting votes.

- uint96 votes = ds.nouns.getPriorVotes(voter, proposalVoteSnapshotBlock(ds, proposalId, proposal));
+ uint96 balance = ds.nouns.balanceOf(voter);
+ if (balance == 0) {
+     revert("Sorry, must own a token to vote");
+ }
+ uint96 votes = ds.nouns.getPriorVotes(voter, proposalVoteSnapshotBlock(ds, proposalId, proposal));

Assessed type

Token-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

eladmallel requested judge review

[c4-sponsor]

eladmallel marked the issue as sponsor disputed

[eladmallel]

this is by design - if you had voting power at the snapshot block you should be able to vote.

perhaps worth nothing that many other DAOs work the same way.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid