createProposal snapshot block can temporarily desync with minApproval / minVotingPower

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L205-L262

Vulnerability details

Impact

minApproval and member list will be temporarily out of sync, potentially causing approval issues

Proof of Concept

https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/plugins/governance/multisig/Multisig.sol#L214-L245

    uint64 snapshotBlock = block.number.toUint64() - 1;

    ...

    // Create the proposal
    Proposal storage proposal_ = proposals[proposalId];

    proposal_.parameters.snapshotBlock = snapshotBlock;
    proposal_.parameters.startDate = _startDate;
    proposal_.parameters.endDate = _endDate;
    proposal_.parameters.minApprovals = multisigSettings.minApprovals;

When creating a proposal all the voting contracts (multisig, token, addresslist) use a snapshot block that is one block in the past. The problem is that they use the current settings for determining min voting are but uses a snapshot block that is 1 block behind. This causes a desync between the members who can vote and the approval needed to pass a proposal.

Example: Imagine the following scenario. There is a multisig with 10 members and a min approval of 6. There is some kind of leak that exposes the private keys of 4 of the members and their addresses are hijacked. The multisig creates a proposal that removes the 4 members and lowers the min approval to 4. On the block that the proposal is executed, one of the malicious members creates a proposal removing the other six members from the multisig. Now when that proposal is created it will use voting eligibility from the block before (which includes the 4 compromised accounts) but it will apply the newly changed min approval of 3. Now the 4 compromised accounts can approve their proposal and hijack the multisig.

Similar scenarios can occur in token and addresslist voting anytime the approval threshold and supply/members are changed in a single proposal.

Tools Used

Manual Review

Recommended Mitigation Steps

Threshold parameters should be snapshot the same way that eligibility or token balances are

[0xean]

Hard to see how this qualifies as H severity. The warden's premise is built off of

There is some kind of leak that exposes the private keys of 4 of the members and their addresses are hijacked.

Which be definition already means that there are security assumptions broken across the entire DAO. I think the point is however valid about the synchronization of these settings.

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[c4-judge]

0xean marked the issue as primary issue

[novaknole20]

Very good finding. Thank you

[c4-sponsor]

novaknole20 marked the issue as sponsor confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report