Skip to content

DAO DAO Incident 0

Jump to bottom
ekez edited this page Aug 2, 2022 · 2 revisions

On the morning 06/05/2022 two bugs were discovered in the DAO DAO v1 single choice proposal contract. At the time, RAW DAO was the only non-testnet deployment of those contracts. A patch was quickly deployed and the RAW DAO upgraded their proposal contract as part of proposal 4.

For the duration of the incident the vulnerability was exploited causing additional funds to be sent to the RAW DAO staking rewards Multisig. These funds were returned to the DAO at the end of the incident in proposal 2. It is unclear if this exploitation was intentional.

No funds have been lost as a result of this incident. Existing DAO DAO DAOs and Multisigs on daodao.zone are not impacted as they do not use the v1 contracts.

Timeline

All times are in PST.

  • 06/05 04:58am: Maxjuno reports an issue executing RAW DAO proposal 3.
  • 06/05 11:02am: Issue is acknowledged by ekez.
  • 06/05 11:05am: Issue is found to be that the DAO's proposal module does not have sufficient $RAW tokens to repay the proposal deposit. The needed $RAW is transferred from a ekez's personal wallet to the proposal module and the proposal is executed.
  • 06/05 12:03pm: Issue is determined to be that proposal 2 was executed more than once in transaction FB03531A7E20568A3AF84BCA604A416281A79B0FFD1CBDF893DC37048BB2A8C8 and transaction 0DAABEC27AF2C16756B3BC2541107D97E094E5311FAB8822DFBBA54D419614CC. RAW DAO development team is alerted.
  • 06/05 01:08pm: A patch for the vulnerability is written and a new version of the proposal contract is uploaded to the Juno mainnet with code ID 282. Patch is tested using a test RAW DAO deployment which matches the one being used.
  • 06/05 02:05pm: Further testing is performed and RAW DAO proposal 4 to upgrade to the new proposal module is put up to vote.
  • 06/06 10:04pm: Proposal passes. Vulnerability is not actively exploitable so actual upgrade is postponed to following day so it occurs during developer waking hours.
  • 06/07 10:21am: Proposal is executed and upgrade is complete. Testing is preformed which confirms that vulnerabilities have been mitigated.
  • 06/07 01:37pm: Funds sent to the RAW DAO staking rewards multisig as a result of the vulnerability are returned to the DAO in transaction 2BEABB99CCBCBB72DBEF2BCF664F0BEE701F33A064DF9DE3DF5FCA8A62EFD672.

Vulnerability analysis

Two vulnerabilities were discovered during the incident:

  1. A passed proposal could be executed more than once.
  2. A passed proposal could be closed if it expired before being executed.

Vulnerability 1. Multiple executions

When a proposal was executed, the contract checked that the proposal had in fact passed, then set the proposal's status to executed:

let old_status = prop.status;
if !prop.is_passed(&env.block) {
    return Err(ContractError::NotPassed {});
}
prop.status = Status::Executed;
PROPOSALS.save(deps.storage, proposal_id, &prop)?;

However, checking only that a proposal is passed before executing it is not a strong enough condition. We must also check that a proposal has not been executed; otherwise, proposals can be executed more than once.

Failing to have this check meant that RAW DAO proposals could be executed more than once for the duration of this incident. While the UI would not present the option to execute a proposal that had already been executed, a malicious actor could do so manually via the CLI. Stale UI state could also allow a non-malicious member of the DAO to cause a proposal to be executed more than once.

To fix this issue, an additional check was added that asserted that the proposal has not already been executed:

if prop.status == Status::Executed {
    return Err(ContractError::AlreadyExecuted {});
}

Vulnerability 2. Closing expired proposals

For quorum-based DAOs, a proposal may pass after it expires. For example, in a DAO with a 10% quorum and majority passing threshold, a proposal could have 10% turnout, the majority of which votes yes. During its voting period, the proposal would not pass, but once the voting period expired, the proposal would pass: it would have met quorum and had a majority of yes votes.

The erroneous logic was this: when checking whether a proposal could be closed, the contract checked only that the proposal was expired:

if !prop.expiration.is_expired(&env.block) {
    return Err(ContractError::WrongCloseStatus {});
}

A patch was deployed, adding the additional check that the proposal in question had not passed:

if !prop.expiration.is_expired(&env.block) || prop.is_passed(&env.block) {
    return Err(ContractError::WrongCloseStatus {});
}

Without this check, an attacker could conduct a denial of service attack on proposals by closing them as soon as their voting period ends. This would prevent a DAO from executing passed proposals.

Forward

The proposal module with this bug has 95% test coverage. Those tests do a good job at verifying that the proposal module correctly performs actions it is designed to do. The proposal module has also been subject to an external audit by Security DAO which similarly did not reveal these issues. By the "book" we did fine.

This experience does a lot to underscore the importance of adversarial testing. We can do better about writing tests that should fail to test both happy and sad execution paths.

These vulnerabilities went unnoticed due to a lack of tests that checked for unallowed actions. Moving forward, it is important that we emphasize testing both that our code behaves correctly and that it does not allow additional actions outside of correct behavior.

Clone this wiki locally