Nouns DAO contest - zzzitron's results

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/c1c7c6201d0247f92472419ff657b570f9104565/contracts/base/ERC721Checkpointable.sol#L126-L144

Vulnerability details

Impact

MED - the function of the protocol or its availability could be impacted

The ERC721Checkpointable::delegateBySig function allows users to delegate to null address. If it happens, the user cannot delegate and transfer anymore.

Proof of Concept

• poc patch from nounsGovernance.test.ts

The snippet below shows the proof of concept, which was modified from the test code nounsGovernance.test.ts. The git diff from the original file can be found here.

In the proof of concept, the deployer delegates to zero address by using delegateBySig. The deployer tries to delegate to another address, but it reverts. The deployer tries to transfer the token but it reverts as well. Therefore, the vote is lost and is not recoverable, even though the token itself is not lost.

If an user had 3 tokens and delegated those to the null address, the user cannot delegate or transfer. If the user acquire 2 more tokens, they cannot delegate, but they still can transfer up to 2 tokens.

// proof of concept to delegate to address(0)
// attempt to delegate to another address and to transfer but fails

    it('delegates to address(0) using delegateBySig poc', async () => {
      await setTotalSupply(token, 1);

      // Delegate from Deployer -> Account1
      await (await tokenCallFromDeployer.delegate(account1.address)).wait();
      await mineBlock();
      await mineBlock();

      expect(await token.getCurrentVotes(address(0))).to.equal(0);
      expect(await token.getCurrentVotes(deployer.address)).to.equal(0);
      expect(await token.getCurrentVotes(account1.address)).to.equal(ONE);

      // Delegate from Deployer -> Address(0), which should assign back to deployer
      // Instead of using delegate, using delegateBySig will delegate to Address(0)

      // await (await tokenCallFromDeployer.delegate(address(0))).wait();
      
      // beging: using delegateBySig to delegate to Address(0)
      var delegatee = address(0),
        nonce = 0,
        expiry = 10e9;
      const signature = await deployer._signTypedData(domain, Types, { delegatee, nonce, expiry });
      var { v, r, s } = ethers.utils.splitSignature(signature);

      const tx = await (await tokenCallFromDeployer.delegateBySig(delegatee, nonce, expiry, v, r, s)).wait();
      // end: using delegateBySig to delegate to Address(0)

      await mineBlock();
      await mineBlock();

      expect(await token.getCurrentVotes(address(0))).to.equal(0);
      // The deployer is supposed to have a vote of ONE
      // expect(await token.getCurrentVotes(deployer.address)).to.equal(ONE);
      // but only zero
      expect(await token.getCurrentVotes(deployer.address)).to.equal(0);
      expect(await token.getCurrentVotes(account1.address)).to.equal(0);

      // Attempt to Delegate from Deployer -> Account1
      // but it reverts
      // await (await tokenCallFromDeployer.delegate(account1.address)).wait();
      await expect(tokenCallFromDeployer.delegate(account1.address)).to.be.revertedWith('ERC721Checkpointable::_moveDelegates: amount underflows');
      await mineBlock();
      await mineBlock();

      expect(await token.getCurrentVotes(address(0))).to.equal(0);
      expect(await token.getCurrentVotes(deployer.address)).to.equal(0);
      // expect(await token.getCurrentVotes(account1.address)).to.equal(ONE);
      expect(await token.getCurrentVotes(account1.address)).to.equal(0);

      // Attempt to Transfer from Deployer -> Account2
      // but it reverts
      // await (
      //   await tokenCallFromDeployer.transferFrom(deployer.address, account2.address, 0)
      // ).wait();
      await expect(
        tokenCallFromDeployer.transferFrom(deployer.address, account2.address, 0)
      ).to.be.revertedWith('ERC721Checkpointable::_moveDelegates: amount underflows');
      await mineBlock();
      await mineBlock();

      expect(await token.getCurrentVotes(address(0))).to.equal(0);
      expect(await token.getCurrentVotes(deployer.address)).to.equal(0);
      expect(await token.getCurrentVotes(account1.address)).to.equal(0);
//      expect(await token.getCurrentVotes(account2.address)).to.equal(ONE);
    });

Cause

The delegateBySig does not check for address zero, unlike delegate function. So, one can set the delegatee to zero address. In the _moveDelegates, if the delegatee is the zero address, it does not write anything, which means the owner does not get the vote in their checkpoint. Next time when the owner of the token tries to delegate, the _delegate tries to subtract from the owner's checkpoint because of the auto-delegation:

// file: ERC721Checkpointable.sol::_delegate
197     function _delegate(address delegator, address delegatee) internal {
198         /// @notice differs from `_delegate()` in `Comp.sol` to use `delegates` override method to simulate auto-delegation
199         address currentDelegate = delegates(delegator);

But when the delegation to the zero address happens, the vote was not wrote to the owner of the token, so it will cause underflow and revert. Since _beforeTokenTransfer calls _moveDelegates, transfer attempt when the vote count is zero will revert.

The relevant functions are in the below snippet:

// file: ERC721Checkpointable.sol

112     function delegate(address delegatee) public {
113         if (delegatee == address(0)) delegatee = msg.sender;
114         return _delegate(msg.sender, delegatee);
115     }

126     function delegateBySig(
127         address delegatee,
128         uint256 nonce,
129         uint256 expiry,
130         uint8 v,
131         bytes32 r,
132         bytes32 s
133     ) public {
134         bytes32 domainSeparator = keccak256(
135             abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
136         );
137         bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
138         bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
139         address signatory = ecrecover(digest, v, r, s);
140         require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
141         require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
142         require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
143         return _delegate(signatory, delegatee);
144     }

197     function _delegate(address delegator, address delegatee) internal {
198         /// @notice differs from `_delegate()` in `Comp.sol` to use `delegates` override method to simulate auto-delegation
199         address currentDelegate = delegates(delegator);
200
201         _delegates[delegator] = delegatee;
202
203         emit DelegateChanged(delegator, currentDelegate, delegatee);
204
205         uint96 amount = votesToDelegate(delegator);
206
207         _moveDelegates(currentDelegate, delegatee, amount);
208     }

210     function _moveDelegates(
211         address srcRep,
212         address dstRep,
213         uint96 amount
214     ) internal {
215         if (srcRep != dstRep && amount > 0) {
216             if (srcRep != address(0)) {
217                 uint32 srcRepNum = numCheckpoints[srcRep];
218                 uint96 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0;
219                 uint96 srcRepNew = sub96(srcRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount underflows');
220                 _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew);
221             }
222
223             if (dstRep != address(0)) {
224                 uint32 dstRepNum = numCheckpoints[dstRep];
225                 uint96 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0;
226                 uint96 dstRepNew = add96(dstRepOld, amount, 'ERC721Checkpointable::_moveDelegates: amount overflows');
227                 _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew);
228             }
229         }
230     }

232     function _writeCheckpoint(
233         address delegatee,
234         uint32 nCheckpoints,
235         uint96 oldVotes,
236         uint96 newVotes
237     ) internal {
238         uint32 blockNumber = safe32(
239             block.number,
240             'ERC721Checkpointable::_writeCheckpoint: block number exceeds 32 bits'
241         );
242
243         if (nCheckpoints > 0 && checkpoints[delegatee][nCheckpoints - 1].fromBlock == blockNumber) {
244             checkpoints[delegatee][nCheckpoints - 1].votes = newVotes;
245         } else {
246             checkpoints[delegatee][nCheckpoints] = Checkpoint(blockNumber, newVotes);
247             numCheckpoints[delegatee] = nCheckpoints + 1;
248         }
249
250         emit DelegateVotesChanged(delegatee, oldVotes, newVotes);
251     }

Tools Used

hardhat

Recommended Mitigation Steps

Add zero address check in the delegateBySig.

// An example mitigation:

126     function delegateBySig(
127         address delegatee,
128         uint256 nonce,
129         uint256 expiry,
130         uint8 v,
131         bytes32 r,
132         bytes32 s
133     ) public {
134         bytes32 domainSeparator = keccak256(
135             abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
136         );
137         bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));
138         bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
139         address signatory = ecrecover(digest, v, r, s);
140         require(signatory != address(0), 'ERC721Checkpointable::delegateBySig: invalid signature');
141         require(nonce == nonces[signatory]++, 'ERC721Checkpointable::delegateBySig: invalid nonce');
142         require(block.timestamp <= expiry, 'ERC721Checkpointable::delegateBySig: signature expired');
+           // Add zero address check before calling `_delegate`
+           if (delegatee == address(0)) delegatee = msg.sender;
143         return _delegate(signatory, delegatee);
144     }

<!-- zzzitron M00 -->

NounsDAO QA Report

Summary

Low

L00: one can set vetoer to zero address without using _burnVetoPower function

Although _burnVetoPower and _setVetoer are separated to prevent accidents of setting the vetoer to the zero address, the _setVetoer allows to set zero address as vetoer. So it does not prevent accidentally setting vetoer to zero address.

To prevent this, write an internal function with the current _setVetoer code and use it in the _setVetoer and _burnVetoPower function. And add zero address check in the _setVetoer function.

// NounsDAOLogicV2.sol

 839     function _setVetoer(address newVetoer) public {
 840         require(msg.sender == vetoer, 'NounsDAO::_setVetoer: vetoer only');
 841
 842         emit NewVetoer(vetoer, newVetoer);
 843
 844         vetoer = newVetoer;
 845     }

 851     function _burnVetoPower() public {
 852         // Check caller is pendingAdmin and pendingAdmin ≠ address(0)
 853         require(msg.sender == vetoer, 'NounsDAO::_burnVetoPower: vetoer only');
 854
 855         _setVetoer(address(0));
 856     }

L01: unused constant value from version 1

The MAX_QUORUM_VOTES_BPS is not used in the NounsDAOLogicV2.

// NounsDAOLogicV2.sol
89     uint256 public constant MAX_QUORUM_VOTES_BPS = 2_000; // 2,000 basis points or 20%

L02: typo / misleading comments

• The below comment does not match the code: the value is 6,000 basis points not 4,000 basis points.

// NounsDAOLogicV2.sol
86     uint256 public constant MAX_QUORUM_VOTES_BPS_UPPER_BOUND = 6_000; // 4,000 basis points or 60%

• The comment below (line 263) says minQuorumVotes is always zero, but minQuorumVotes() is not always zero. (newProposal.quorumVotes is always zero). The minQuorumVotes() is using information from the current block, so it is not final: it can be overwritten.

// NounsDAOLogicV2.sol
 262         /// @notice Updated event with `proposalThreshold` and `minQuorumVotes`
 263         /// @notice `minQuorumVotes` is always zero since V2 introduces dynamic quorum with checkpoints
 264         emit ProposalCreatedWithRequirements(
 265             newProposal.id,
 266             msg.sender,
 267             targets,
 268             values,
 269             signatures,
 270             calldatas,
 271             newProposal.startBlock,
 272             newProposal.endBlock,
 273             newProposal.proposalThreshold,
 274             minQuorumVotes(),
 275             description
 276         );

<!-- zzzitron QA -->