Skip to content

Zero Address Delegation#

Medium Risk

Vulnerability#

If a user delegates their votes to the zero address, their votes will be lost and root tokens locked in the wrapper contract forever.

This edge case occurs due to auto-delegation in ERC721WrapperVotes.sol contract logic when the delegate is unset. Under certain circumstances described below, this can cause reverts due to underflow. The logic is somewhat nuanced so we also provide a test case below to demonstrate the undesirable behavior:

Test Case#

it('Disallows unwrap after delegating to the zero address (test passing demos undesirable behavior)', async function () {
  await testNFT.mint(accounts[0].address, 123);

  // wrap
  await testNFT['safeTransferFrom(address,address,uint256)'](
    accounts[0].address,
    erc721WrapperVotes.address,
    123
  );

  // unwrap works initially
  await expect(erc721WrapperVotes.unwrap(123)).to.not.be.reverted;

  // wrap again
  await testNFT['safeTransferFrom(address,address,uint256)'](
    accounts[0].address,
    erc721WrapperVotes.address,
    123
  );

  const votesBefore = await erc721WrapperVotes.getVotes(accounts[0].address);

  await erc721WrapperVotes.delegate(ethers.constants.AddressZero);

  const votes = await erc721WrapperVotes.getVotes(accounts[0].address);

  expect(votesBefore).to.equal(1);
  expect(votes).to.equal(0);

  // reverts with underflow
  await expect(erc721WrapperVotes.unwrap(123)).to.be.reverted;

  // attempt to delegate to self again (essentially no-op)
  await erc721WrapperVotes.delegate(accounts[0].address);

  // attempt to delegate to another address (reverts)
  await expect(erc721WrapperVotes.delegate(accounts[1].address)).to.be.reverted;

  // still reverts with underflow, token is locked in the contract
  await expect(erc721WrapperVotes.unwrap(123)).to.be.reverted;
});

Auto Delegating#

If the user has not delegated votes, the contract logic interprets this as delegating to themselves:

/**
  * @dev Returns the delegate that `account` has chosen.
  */
function delegates(address account) public view virtual returns (address) {
    if (_delegation[account] == address(0)) {
        return account;
    }
    return _delegation[account];
}

Delegate Function#

/**
  * @dev Delegate all of `account`'s voting units to `delegatee`.
  *
  * Emits events {DelegateChanged} and {DelegateVotesChanged}.
  */
function _delegate(address account, address delegatee) internal virtual {
    address oldDelegate = delegates(account);
    _delegation[account] = delegatee;

    emit DelegateChanged(account, oldDelegate, delegatee);
    _moveDelegateVotes(
        oldDelegate,
        delegatee,
        uint16(userInfos[account].balance)
    );
}

/**
  * @dev Moves delegated votes from one delegate to another.
  */
function _moveDelegateVotes(
    address from,
    address to,
    uint16 amount
) private {
    if (from != to && amount > 0) {
        if (from != address(0)) {
            uint16 oldValue = _delegateCheckpoints[from].push(
                _subtract,
                amount
            );
            emit DelegateVotesChanged(from, oldValue, oldValue - amount);
        }
        if (to != address(0)) {
            uint16 oldValue = _delegateCheckpoints[to].push(_add, amount);
            emit DelegateVotesChanged(to, oldValue, oldValue + amount);
        }
    }
}

When a user delegates to the zero address, due to auto-delegation oldDelegate will always be interpreted to be a non-zero address, while delegatee is still the zero address. As a result, the oldDelegate votes are removed, and no votes are added to the zero address (which is expected).

Once the user has delegated to the zero address, however, their own balance of votes will be zero in _delegateCheckpoints state. When they try to delegate or unwrap their tokens, the oldDelegate is still interpreted as non zero via solidity address oldDelegate = delegates(account); and checkpointing variable underflows when attempting to subtract from _delegateCheckpoints[oldDelegate] which has a zero balance.

As a result, unwrapping or re-delegating to another address result in reverts and the votes / tokens are essentially lost.

Exploit Example#

This bug can be exploited by attackers who wish to effectively burn tokens or otherwise manipulate governance by censoring certain voters. Burning tokens also reduces the NFT supply which due to scarcity can increase the value of the asset for other token holders. What is more, because this exploit involves interacting directly with the trusted governance contract, misuse and phishing attacks are more feasible.

An example scenario:

  1. An important and controversial governance proposal is created with high stakes for participants
  2. Malicious actor solicits participants to "delegate to themselves" by clearing their delegation to "ensure their votes are appropriately counted". They may even point to the contract logic which reinforces the idea of "auto-delegation" if your delegation is set to the zero address.
  3. Victims tokens are effectively burned and are unable to participate in governance.

A malicious governance UI can accomplish this sort of attack as well via a similar "signature hoarding" strategy as described in the Signature Replay Vulnerability entry.

Recommendation#

Disallow explicit delegation to the zero address in the delegation logic. If a user wishes to delegate to themselves they should delegate to their own address. Note that initial auto-delegation will still work with this check in place.

require(delegatee != address(0), "Cannot delegate to zero address")

Additional Recommendation#

The plural delegates(address) function implies there can be multiple delegates, consider changing the name to something like delegateOf(address), which more clearly describes the functionality.