code4rena-2022-08-nounsdao-m02

[M-02] User A cannot cancel User B’s proposal when User B’s prior number of votes at relevant block is same as proposal threshold, which contradicts the fact that User B actually cannot create the proposal when the prior number of votes is same as proposal threshold

Summary

숫자 비교 시 edge case 고려를 하지 않아 로직에 결함이 생겼다.

Keyword

logic flaw, edge case

Vulnerability

    function propose(
        address[] memory targets,
        uint256[] memory values,
        string[] memory signatures,
        bytes[] memory calldatas,
        string memory description
    ) public returns (uint256) {
        ProposalTemp memory temp;
 
        temp.totalSupply = nouns.totalSupply();
 
        temp.proposalThreshold = bps2Uint(proposalThresholdBPS, temp.totalSupply);
 
        require(
            nouns.getPriorVotes(msg.sender, block.number - 1) > temp.proposalThreshold,
            'NounsDAO::propose: proposer votes below proposal threshold'
        );
        require(
            targets.length == values.length &&
                targets.length == signatures.length &&
                targets.length == calldatas.length,
            'NounsDAO::propose: proposal function information arity mismatch'
        );
        require(targets.length != 0, 'NounsDAO::propose: must provide actions');
        require(targets.length <= proposalMaxOperations, 'NounsDAO::propose: too many actions');
 
        temp.latestProposalId = latestProposalIds[msg.sender];
        if (temp.latestProposalId != 0) {
            ProposalState proposersLatestProposalState = state(temp.latestProposalId);
            require(
                proposersLatestProposalState != ProposalState.Active,
                'NounsDAO::propose: one live proposal per proposer, found an already active proposal'
            );
            require(
                proposersLatestProposalState != ProposalState.Pending,
                'NounsDAO::propose: one live proposal per proposer, found an already pending proposal'
            );
        }
 
        temp.startBlock = block.number + votingDelay;
        temp.endBlock = temp.startBlock + votingPeriod;
 
        proposalCount++;
        Proposal storage newProposal = _proposals[proposalCount];
        newProposal.id = proposalCount;
        newProposal.proposer = msg.sender;
        newProposal.proposalThreshold = temp.proposalThreshold;
        newProposal.eta = 0;
        newProposal.targets = targets;
        newProposal.values = values;
        newProposal.signatures = signatures;
        newProposal.calldatas = calldatas;
        newProposal.startBlock = temp.startBlock;
        newProposal.endBlock = temp.endBlock;
        newProposal.forVotes = 0;
        newProposal.againstVotes = 0;
        newProposal.abstainVotes = 0;
        newProposal.canceled = false;
        newProposal.executed = false;
        newProposal.vetoed = false;
        newProposal.totalSupply = temp.totalSupply;
        newProposal.creationBlock = block.number;
 
        latestProposalIds[newProposal.proposer] = newProposal.id;
 
        /// @notice Maintains backwards compatibility with GovernorBravo events
        emit ProposalCreated(
            newProposal.id,
            msg.sender,
            targets,
            values,
            signatures,
            calldatas,
            newProposal.startBlock,
            newProposal.endBlock,
            description
        );
 
        /// @notice Updated event with `proposalThreshold` and `minQuorumVotes`
        /// @notice `minQuorumVotes` is always zero since V2 introduces dynamic quorum with checkpoints
        emit ProposalCreatedWithRequirements(
            newProposal.id,
            msg.sender,
            targets,
            values,
            signatures,
            calldatas,
            newProposal.startBlock,
            newProposal.endBlock,
            newProposal.proposalThreshold,
            minQuorumVotes(),
            description
        );
 
        return newProposal.id;
    }

유저 B가 새로운 Proposal을 생성하려면 전체 NFT의 n%만큼을 소유하고 있어야 한다. 최소 0.01% ~ 최대 10% 로 admin이 설정 가능한 값이다.

temp.proposalThreshold = bps2Uint(proposalThresholdBPS, temp.totalSupply); 로 새 proposal을 생성하기 위해 필요한 NFT 수를 구함
require(nouns.getPriorVotes(msg.sender, block.number - 1) > temp.proposalThreshold 'NounsDAO::propose: proposer votes below proposal threshold'); 로 직전까지 소유했던 NFT 개수가 충분한지 확인한다.

유저의 NFT 개수와 proposalThreshold 를 비교할 때 >= 가 아니라 > 임에 주목하자. proposalThreshold 와 동일한 수의 NFT를 가지고 있는 경우, proposal을 생성할 수 없다.

다음은 cancel 함수를 살펴보자. cancel 함수를 이용하면 proposal을 제시한 유저 B가 자신의 proposal을 취소할 수 있다. 또한, 유저 B가 더이상 proposalThreshold 를 만족할 수 없는 경우 다른 유저(유저 A)가 아직 완료되지 않은 유저 B의 proposal을 취소시킬 수 있다. proposal을 생성한 후 NFT를 옮기는 등 악용하는 것을 방지하기 위해서이다.

    function cancel(uint256 proposalId) external {
        require(state(proposalId) != ProposalState.Executed, 'NounsDAO::cancel: cannot cancel executed proposal');
 
        Proposal storage proposal = _proposals[proposalId];
        require(
            msg.sender == proposal.proposer ||
                nouns.getPriorVotes(proposal.proposer, block.number - 1) < proposal.proposalThreshold,
            'NounsDAO::cancel: proposer above threshold'
        );
 
        proposal.canceled = true;
        for (uint256 i = 0; i < proposal.targets.length; i++) {
            timelock.cancelTransaction(
                proposal.targets[i],
                proposal.values[i],
                proposal.signatures[i],
                proposal.calldatas[i],
                proposal.eta
            );
        }
 
        emit ProposalCanceled(proposalId);
    }

require(msg.sender == proposal.proposer || nouns.getPriorVotes(proposal.proposer, block.number - 1) < proposal.proposalThreshold, 'NounsDAO::cancel: proposer above threshold'); 코드에서 proposer의 현재 NFT 수를 구하고, 이것을 proposal 제시 시점의 proposalThreshold 와 비교한다.

이 때, <= 가 아니라 < 로 비교함에 주목하자. 즉, cancel 시에 유저 B가 proposal.proposalThreshold 와 동일한 수의 NFT를 가지고 있는 경우, 유저 A가 proposal을 삭제할 수 없다. 즉, 생성시와는 다르게 proposalThreshold 와 동일한 개수의 NFT를 소유한 경우 proposal을 제시할 자격이 있다고 판단되는 것이다. cancel과 propose 함수의 기준이 불일치한다.

따라서 유저 B가 proposal을 생성한 뒤, NFT를 옮겨 proposalThreshold 와 딱 맞춘다면 당시 proposal을 생성할 자격을 잃음에도 불구하고 타 유저로부터 cancel 당하지도 않는 상태가 된다.

참고로 투표권을 세는 것은 proposal 생성 시점 전의 NFT 소유 여부이므로 해당 proposal에 대한 조작은 불가하다. 하지만 로직 자체가 틀린것으로 취약점은 맞다.

다음은 PoC 이다.

  it("User A cannot cancel User B's proposal when User B's prior number of votes at relevant block is same as proposal threshold, which contradicts the fact that User B actually cannot create the proposal when the prior number of votes is same as proposal threshold",
    async () => {
    // account1 has 3 tokens at the beginning
    // account1 gains 2 more to own 5 tokens in total
    await token.transferFrom(deployer.address, account1.address, 11);
    await token.transferFrom(deployer.address, account1.address, 12);
 
    await mineBlock();
 
    // account1 cannot create a proposal when owning 5 tokens in total
    await expect(
      gov.connect(account1).propose(targets, values, signatures, callDatas, 'do nothing'),
    ).to.be.revertedWith('NounsDAO::propose: proposer votes below proposal threshold');
 
    // account1 gains 1 more to own 6 tokens in total
    await token.transferFrom(deployer.address, account1.address, 13);
 
    await mineBlock();
 
    // account1 can create a proposal when owning 6 tokens in total
    await gov.connect(account1).propose(targets, values, signatures, callDatas, 'do nothing');
    const proposalId = await gov.latestProposalIds(account1.address);
    expect(await gov.state(proposalId)).to.equal(0);
 
    // other user cannot cancel account1's proposal at this moment
    await expect(
      gov.cancel(proposalId, {gasLimit: 1e6})
    ).to.be.revertedWith('NounsDAO::cancel: proposer above threshold');
    
    // account1 removes 1 token to own 5 tokens in total
    await token.connect(account1).transferFrom(account1.address, deployer.address, 13);
 
    await mineBlock();
 
    // other user still cannot cancel account1's proposal when account1 owns 5 tokens in total
    // this contradicts the fact that account1 cannot create a proposal when owning 5 tokens in total
    await expect(
      gov.cancel(proposalId, {gasLimit: 1e6})
    ).to.be.revertedWith('NounsDAO::cancel: proposer above threshold');
 
    // account1 removes another token to own 4 tokens in total
    await token.connect(account1).transferFrom(account1.address, deployer.address, 12);
 
    await mineBlock();
 
    // other user can now cancel account1's proposal when account1 owns 4 tokens in total
    await gov.cancel(proposalId, {gasLimit: 1e6})
    expect(await gov.state(proposalId)).to.equal(2);
  });

Impact

로직이 의도한 대로 동작하지 않는다. proposal을 생성한 뒤, NFT를 옮겨 proposalThreshold 와 딱 맞춘다면 당시 proposal을 생성할 자격은 잃음에도 불구하고 타 유저로부터 cancel 당하지도 않는 상태가 된다.

Mitigation

propose나 cancel 함수 중 한쪽만 변경하여 양측의 조건이 일치하도록 한다.

propose 함수에서 >= 로 비교하여 proposalThreshold 와 동일한 수의 NFT를 소유했을 때에도 proposal을 생성할 수 있도록 변경하거나
cancel 함수에서 <= 로 비교하여 proposalThreshold 와 동일한 수의 NFT를 소유했을 때에 cancel이 가능하도록 수정한다.

tags: bughunting, nouns dao, smart contract, solidity, logic flaw, edge case, dao, severity medium

Digital garden

Explorer

code4rena-2022-08-nounsdao-m02

code4rena-2022-08-nounsdao-m02

[M-02] User A cannot cancel User B’s proposal when User B’s prior number of votes at relevant block is same as proposal threshold, which contradicts the fact that User B actually cannot create the proposal when the prior number of votes is same as proposal threshold

Summary

Keyword

Vulnerability

Impact

Mitigation

Graph View

Table of Contents