Nouns DAO contest - Respx's results

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L518-L524 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L533-L544 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L98

Vulnerability details

Voters can burn large amounts of Ether by submitting votes with long reason strings

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L518-L524 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L533-L544 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L98

Description

The only limits to how long a string argument to a function call can be is the block gas limit of the EVM, currently 30 million. It is therefore possible for a user to call NounsDAOLogicV2.castRefundableVoteWithReason() with a very, very long reason string. castRefundableVoteInternal() emits an event that includes reason on line 540, which is within the region of code covered by gas refunds (gas refunds are measured from startGas on line 538). Because of this, gas refunds will include the gas price of emitting this event, which could potentially be very large.

Impact

This issue is partially mitigated by the fact that the voter will still bear the cost of the massive calldata usage. NounsDAOLogicV2 covers this with a fixed value of REFUND_BASE_GAS (36000), but the real transaction overhead is far larger when submitting a reason string that is many thousands of characters in length. Therefore, the voter ends up losing roughly as much as is drained from the NounsDAOLogicV2 contract by the refund. Nonetheless, I still think this is a valid high funding as the protocol will not want to rely purely on this economic protection. Some risk scenarios:

1. It is quite possible that calldata prices could decrease in future, perhaps as part of catering for rollups. This could make the attack suddenly far more viable.
2. A voter might have some motive to want to emit some arbitrary text as an Ethereum event, and simply exploit this system to do so.
3. A voter might want to maliciously drain the Ether, perhaps to damage the protocol's reputation.
4. As in 3, this could be achieved by emptying out the last funds in ``NounsDAOLogicV2` and so denying many other voters their voting refunds.

Tools Used

Code inspection, hardhat testing

Recommended Mitigation Steps

2 alternative ideas:

1. Move the emit VoteCast outside of the gas calculation region of the code and increase REFUND_BASE_GAS to cover an event with a reasonable length of string.
2. Change the type of reason to bytes and add a check to its length in castRefundableVoteWithReason(), reverting if it is too long.

Proof of Concept

The single vote in this test burns around 0.25 Ether from the NounsDAOLogicV2 contract. This test runs slowly and is assuming a base fee of 500 gwei. Obviously if the base fee were higher, the gas burnt would also be higher. The numbers are printed out with a rather messy console.log() in the middle of the test output. Apologies for the bad presentation, but on the bright side you can adjust the numbers and see different results.

diff --git a/hardhat.config.ts b/hardhat.config.ts
index 6d469b0..dc51148 100644
--- a/hardhat.config.ts
+++ b/hardhat.config.ts
@@ -34,7 +34,7 @@ const config: HardhatUserConfig = {
         : [process.env.WALLET_PRIVATE_KEY!].filter(Boolean),
     },
     hardhat: {
-      initialBaseFeePerGas: 0,
+      initialBaseFeePerGas: 500_000_000_000,
     },
   },
   etherscan: {
@@ -50,12 +50,12 @@ const config: HardhatUserConfig = {
   gasReporter: {
     enabled: !process.env.CI,
     currency: 'USD',
-    gasPrice: 50,
+    gasPrice: 500,
     src: 'contracts',
     coinmarketcap: '7643dfc7-a58f-46af-8314-2db32bdd18ba',
   },
   mocha: {
-    timeout: 60_000,
+    timeout: 600_000,
   },
 };
 export default config;

diff --git a/test/governance/NounsDAO/V2/voteRefund.test.ts b/test/governance/NounsDAO/V2/voteRefundPOC.test.ts
index d34ff4b..4c268a3 100644
--- a/test/governance/NounsDAO/V2/voteRefund.test.ts
+++ b/test/governance/NounsDAO/V2/voteRefundPOC.test.ts
@@ -162,6 +162,30 @@ describe('Vote Refund', () => {
   });
 
   describe('castRefundableVoteWithReason', () => {
+    it("accepts excessively long reason strings", async () => {
+      await fundGov();
+      const balanceBefore = await user.getBalance();
+      const govBalanceBefore = await ethers.provider.getBalance(gov.address);
+      const tx = await gov
+        .connect(user)
+        .castRefundableVoteWithReason(1, 1, junkString(50_000), {
+          maxPriorityFeePerGas: MAX_PRIORITY_FEE_CAP,
+          gasLimit: 24000000,
+        });
+      const r = await tx.wait();
+      const balanceDiff = balanceBefore.sub(await user.getBalance());
+      const govBalanceDiff = govBalanceBefore.sub(
+        await ethers.provider.getBalance(gov.address)
+      );
+      const govBalanceAfter = await ethers.provider.getBalance(gov.address);
+      console.log("USER BALANCE DIFF:", ethers.utils.formatEther(balanceDiff));
+      console.log(
+        "GOV BALANCE DIFF:",
+        ethers.utils.formatEther(govBalanceDiff)
+      );
+      console.log("TX COST:", ethers.utils.formatEther(await txCostInEth(r)));
+
+    });
     it('refunds users with votes', async () => {
       await fundGov();
       const balanceBefore = await user.getBalance();
@@ -284,6 +308,15 @@ describe('Vote Refund', () => {
     expect(refundEvent!.args!.refundAmount).to.be.closeTo(expectedCost, REFUND_ERROR_MARGIN);
   }
 
+  function junkString(iterations: number = 100) {
+    var x = "Ab Cd Ef Gh Ij ";
+    const y = "Ab Cd Ef Gh Ij";
+    for (var i = 0; i < iterations; i++) {
+      x += y;
+    }
+    return x;
+  }
+
   async function submitProposal(u: SignerWithAddress) {
     await gov
       .connect(u)

Summary

This code is very well presented in my opinion, with good comments. There are some functions without comments (see below) and it could be improved by more comments within the functions explaining the code.

Non Critical

(code style, clarity, syntax, versioning, off-chain monitoring (events, etc)

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L94 NounsDAOLogicV1 line 94: The constant proposalMaxOperations should be in all caps like the other constants or, if there is a good reason for it not to be, that reason should be given in a comment.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L92 NounsDAOLogicV2 line 92 The constant proposalMaxOperations should be in ALL CAPS like the other constants or, if there is a good reason for it not to be, that reason should be explained in a comment.

Low

(e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments)

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L46 NounsDAOLogicV1 line 46: comment typo: "veteor" should be "vetoer".

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L336 NounsDAOLogicV1.cancel() can be called on any proposal that is not executed. That means it is possible to call it on proposals that have a status which is already concluded: Defeated, Expired, Vetoed (assuming that timelock.cancelTransaction() calls this function, it will not revert, and so the status change will succeed). For Defeated or Expired, it will update the reported status of the propsal, misrepresenting the reason the proposal was not executed and possibly disrupting data and reporting tools.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L336 NounsDAOLogicV1.veto() can be called on any proposal that is not executed. That means it is possible to call it on proposals that have a status which is already concluded: Defeated, Expired, Canceled (assuming that timelock.cancelTransaction() calls this function, it will not revert, and so the status change will succeed). This will update the reported status of the propsal, misrepresenting the reason the proposal was not executed and possibly disrupting data and reporting tools.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L86 Comment on line 86 of NounsDAOLogicV2 is wrong: 6,000 basis points, not 4,000.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L294 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L672 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L676 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L305 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#783 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#866 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#964 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#974 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#988 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#1006 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#1010 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#1018 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#1023 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L197 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L210 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L232 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L253 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L258 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L263 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L273 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/base/ERC721Checkpointable.sol#L282 The above linked functions are all missing comment headers to explain their purpose and give any other pertinent details.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#1030 I would add a comment explaining why the contract needs to be payable now.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L263-L274 The comment on line 263 of NounsDAOLogicV2.sol states that minQuorumVotes() will always return zero. So it would be more gas efficient to simply replace that call with the literal value 0.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L107 If the contract size of NounsDAOLogicV2 is too large, one obvious opportunity to save space is the many lines such as 134,622,638,655,674,702,727,801 which are checking require(msg.sender == admin). These could all be replaced by a modifier making the same check. This would lose a few custom error strings, but they simply name the function that was called: there is no real loss of utility.

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L676-L683 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L1010-L1016 getChainIdInternal() in both NounsDAOLogicV1 and NounsDAOLogicV2 is more gas efficient if rewritten as chainTest2() below:

contract ChainTest {

    function chainTest1() external view returns (uint256) {
        uint256 chainId;
        assembly {
            chainId := chainid()
        }
        return chainId;
    }

    function chainTest2() external view returns (uint256 chainId) {
      assembly {
        chainId := chainid()
      }
  }
  
}

https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L535-L538 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L627-L630 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L551-L554 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L643-L646 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L569-L572 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L661-L664 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV1.sol#L586-L589 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L687-L692 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L714-L719 https://github.com/code-423n4/2022-08-nounsdao/blob/45411325ec14c6d747b999a40367d3c5109b5a89/contracts/governance/NounsDAOLogicV2.sol#L730-L735 Affected functions:

NounsDAOLogicV1._setVotingDelay()
NounsDAOLogicV2._setVotingDelay()
NounsDAOLogicV1._setVotingPeriod()
NounsDAOLogicV2._setVotingPeriod()
NounsDAOLogicV1._setProposalThresholdBPS()
NounsDAOLogicV2._setProposalThresholdBPS()
NounsDAOLogicV1._setQuorumVotesBPS()
NounsDAOLogicV2._setMinQuorumVotesBPS()
NounsDAOLogicV2._setMaxQuorumVotesBPS()
NounsDAOLogicV2._setQuorumCoefficient()

Test contract:

contract VotingDelaySetTest {
    uint public votingDelay = 123_456;
    event VotingDelaySet(uint256 oldVotingDelay, uint256 newVotingDelay);

    function setVotingDelay1(uint256 newVotingDelay) external {
        uint256 oldVotingDelay = votingDelay;
        votingDelay = newVotingDelay;
        emit VotingDelaySet(oldVotingDelay, votingDelay);
    }

    function setVotingDelay2(uint256 newVotingDelay) external {
        emit VotingDelaySet(votingDelay, newVotingDelay);
        votingDelay = newVotingDelay;
    }
}

The listed functions all have variable updates and events in the format shown in the test contract in setVotingDelay1(). Switching this round to emit the event first and then updating, without using the memory variable, as in setVotingDelay2(), gave consistent lower gas costs.