Nouns DAO - cccz's results

Lines of code

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/NounsDAOV3Fork.sol#L95-L102

Vulnerability details

Impact

During the escrow period, users can escrow to or withdraw from forkEscrow their Nouns. During the escrow period, proposals can be executed.

    function withdrawFromForkEscrow(NounsDAOStorageV3.StorageV3 storage ds, uint256[] calldata tokenIds) external {
        if (isForkPeriodActive(ds)) revert ForkPeriodActive();

        INounsDAOForkEscrow forkEscrow = ds.forkEscrow;
        forkEscrow.returnTokensToOwner(msg.sender, tokenIds);

        emit WithdrawFromForkEscrow(forkEscrow.forkId(), msg.sender, tokenIds);
    }

Since withdrawFromForkEscrow will only call the returnTokensToOwner function of ds.forkEscrow, and returnTokensToOwner is only allowed to be called by DAO. If, during the escrow period, ds.forkEscrow is changed by the proposal's call to _setForkEscrow, then the user's escrowed Nouns will not be withdrawn by withdrawFromForkEscrow.

    function returnTokensToOwner(address owner, uint256[] calldata tokenIds) external onlyDAO {
        for (uint256 i = 0; i < tokenIds.length; i++) {
            if (currentOwnerOf(tokenIds[i]) != owner) revert NotOwner();

            nounsToken.transferFrom(address(this), owner, tokenIds[i]);
            escrowedTokensByForkId[forkId][tokenIds[i]] = address(0);
        }

        numTokensInEscrow -= tokenIds.length;
    }

Consider that some Nouners is voting on a proposal that would change ds.forkEscrow. There are some escrowed Nouns in forkEscrow (some Nouners may choose to always escrow their Nouns to avoid missing fork). The proposal is executed, ds.forkEscrow is updated, and the escrowed Nouns cannot be withdrawn.

Proof of Concept

https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/NounsDAOV3Fork.sol#L95-L102 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/fork/NounsDAOForkEscrow.sol#L116-L125 https://github.com/nounsDAO/nouns-monorepo/blob/718211e063d511eeda1084710f6a682955e80dcb/packages/nouns-contracts/contracts/governance/NounsDAOV3Admin.sol#L527-L531

Tools Used

None

Recommended Mitigation Steps

Consider allowing the user to call forkEscrow.returnTokensToOwner directly to withdraw escrowed Nouns, and need to move isForkPeriodActive from withdrawFromForkEscrow to returnTokensToOwner.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

cancel() does not allow to cancel proposals in the final states Canceled/Defeated/Expired/Executed/Vetoed.

    function cancel(NounsDAOStorageV3.StorageV3 storage ds, uint256 proposalId) external {
        NounsDAOStorageV3.ProposalState proposalState = stateInternal(ds, proposalId);
        if (
            proposalState == NounsDAOStorageV3.ProposalState.Canceled ||
            proposalState == NounsDAOStorageV3.ProposalState.Defeated ||
            proposalState == NounsDAOStorageV3.ProposalState.Expired ||
            proposalState == NounsDAOStorageV3.ProposalState.Executed ||
            proposalState == NounsDAOStorageV3.ProposalState.Vetoed
        ) {
            revert CantCancelProposalAtFinalState();
        }

The Canceled/Executed/Vetoed states are final because they cannot be changed once they are set. The Defeated state is also a final state because no new votes will be cast (stateInternal() may return Defeated only if the objectionPeriodEndBlock is passed).

But the Expired state depends on the GRACE_PERIOD of the timelock, and GRACE_PERIOD may be changed due to upgrades. Once the GRACE_PERIOD of the timelock is changed, the state of the proposal may also be changed, so Expired is not the final state.

        } else if (block.timestamp >= proposal.eta + getProposalTimelock(ds, proposal).GRACE_PERIOD()) {
            return NounsDAOStorageV3.ProposalState.Expired;
        } else {
            return NounsDAOStorageV3.ProposalState.Queued;

Consider the following scenario. Alice submits proposal A to stake 20,000 ETH to a DEFI protocol, and it is successfully passed, but it cannot be executed because there is now only 15,000 ETH in the timelock (consumed by other proposals), and then proposal A expires. The DEFI protocol has been hacked or rug-pulled. Now proposal B is about to be executed to upgrade the timelock and extend GRACE_PERIOD (e.g., GRACE_PERIOD is extended by 7 days from V1 to V2). Alice wants to cancel Proposal A, but it cannot be canceled because it is in Expired state. Proposal B is executed, causing Proposal A to change from Expired to Queued. The malicious user sends 5000 ETH to the timelock and immediately executes Proposal A to send 20000 ETH to the hacked protocol.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider adding a proposal expiration time field in the Proposal structure

    function queue(NounsDAOStorageV3.StorageV3 storage ds, uint256 proposalId) external {
        require(
            stateInternal(ds, proposalId) == NounsDAOStorageV3.ProposalState.Succeeded,
            'NounsDAO::queue: proposal can only be queued if it is succeeded'
        );
        NounsDAOStorageV3.Proposal storage proposal = ds._proposals[proposalId];
        INounsDAOExecutor timelock = getProposalTimelock(ds, proposal);
        uint256 eta = block.timestamp + timelock.delay();
        for (uint256 i = 0; i < proposal.targets.length; i++) {
            queueOrRevertInternal(
                timelock,
                proposal.targets[i],
                proposal.values[i],
                proposal.signatures[i],
                proposal.calldatas[i],
                eta
            );
        }
        proposal.eta = eta;
+       proposal.exp = eta + timelock.GRACE_PERIOD();
...
-       } else if (block.timestamp >= proposal.eta + getProposalTimelock(ds, proposal).GRACE_PERIOD()) {
+       } else if (block.timestamp >= proposal.exp) {
            return NounsDAOStorageV3.ProposalState.Expired;

Assessed type

Context