code-423n4 / 2023-07-nounsdao-findings

6 stars 3 forks source link

A proposer can initiate several proposals at once through delegations #241

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Proposals.sol#L160-L164

Vulnerability details

Impact

Within the NounsDAOV3Proposals' propose() function, a proposer is only permitted to have one active proposal at a time. Despite this, a proposer has the capability to delegate to various accounts, enabling these accounts to propose on their behalf.

Proof of Concept

The function checkNoActiveProp() ensures that the proposer does not have an ongoing proposal.

    function checkNoActiveProp(NounsDAOStorageV3.StorageV3 storage ds, address proposer) internal view {
        ...
        if (
            proposersLatestProposalState == NounsDAOStorageV3.ProposalState.ObjectionPeriod ||
            proposersLatestProposalState == NounsDAOStorageV3.ProposalState.Active ||
            proposersLatestProposalState == NounsDAOStorageV3.ProposalState.Pending ||
            proposersLatestProposalState == NounsDAOStorageV3.ProposalState.Updatable
        ) revert ProposerAlreadyHasALiveProposal();
        ...
    }

Additionally, a proposer can delegate their voting power to other accounts by invoking the delegate() function.

However, this capability could potentially lead to the concentration of power. With a single proposer able to delegate their voting power to several accounts, those with high voting power could control the decision-making process. This scenario can be problematic, particularly if the most powerful parties have conflicting interests or objectives divergent from the DAO's goals.

Moreover, the capability to delegate in this manner could lead to proposals being made without adequate consideration or discourse, as delegated accounts might not share the proposer's level of insight or interest in the proposal. Consequently, decisions might be made that do not reflect the DAO members' best interests.

In conclusion, although delegation can enhance DAO participation and engagement, it must be applied judiciously, with sufficient controls to prevent potential adverse outcomes, such as the excessive concentration of power.

    function test_delegate() public {
        vm.prank(address(timelock));
        dao._setProposalThresholdBPS(1000); // 10%
        console2.log("before executing fork, proposal threshold is 2");
        assertEq(dao.proposalThreshold(), 2);
        console2.log("start executing forking");
        dao.executeFork();

        // there are 20 tokens, but 5 are now the DAO's, so adjusted total supply is 15
        assertEq(dao.adjustedTotalSupply(), 15);
        console2.log("after executing fork, , proposal threshold is 1");
        assertEq(dao.proposalThreshold(), 1);

        address Evil = makeAddr("Evil");
        console2.log("holder transfers Noun#13 to Evil");
        vm.prank(tokenHolder);
        nounsToken.transferFrom(tokenHolder, Evil, 13);
        vm.roll(block.number + 1);
        console2.log("holder transfers Noun#14 to Evil");
        vm.prank(tokenHolder);
        nounsToken.transferFrom(tokenHolder, Evil, 14);
        vm.roll(block.number + 1);
        uint256 proposalId;
        proposalId = propose(Evil, address(0), 0, '0', '0', '0');
        console2.log("Evil initiated a proposal with id = %d", proposalId);

        vm.roll(block.number + 1);
        address Bob = makeAddr("Bob");
        console2.log("Evil delegates Bob");
        vm.prank(Evil);
        nounsToken.delegate(Bob);
        vm.roll(block.number + 1);
        proposalId = propose(Bob, address(1), 1, '1', '1', '1');
        console2.log("Bob initiated a proposal with id = %d", proposalId);

        vm.roll(block.number + 1);
        address Tom = makeAddr("Tom");
        console2.log("Evil delegates Tom");
        vm.prank(Evil);
        nounsToken.delegate(Tom);
        vm.roll(block.number + 1);
        proposalId = propose(Tom, address(2), 2, '2', '2', '2');
        console2.log("Tom initiated a proposal with id = %d", proposalId);

    }

Output:

[PASS] test_delegate() (gas: 1897737)
Logs:
  before executing fork, proposal threshold is 2
  start  executing forking
  after  executing fork, , proposal threshold is 1
  holder transfers Noun#13 to Evil
  holder transfers Noun#14 to Evil
  Evil initiated a proposal with id = 1
  Evil delegates Bob
  Bob initiated a proposal with id = 2
  Evil delegates Tom
  Tom initiated a proposal with id = 3

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

Tools Used

Manual

Recommended Mitigation Steps

To address the potential issue with delegation in the DAO, some recommendations are include:

By implementing these recommendations, the DAO can help to ensure that vote delegation is used in a responsible and effective manner, while minimizing the potential risks associated with concentration of power and uninformed decision-making.

Assessed type

Governance

0xSorryNotSorry commented 1 year ago

With a single proposer able to delegate their voting power to several accounts, those with high voting power could control the decision-making process.

The above quote is not true as the delegator's voting power is removed for the each delegatee that the votes are delegated for.

Invalid assumption.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as low quality report

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid