Revolution Protocol - BowTiedOriole's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/base/VotesUpgradeable.sol#L164-L167

Vulnerability details

Bug Description

VotesUpgradeable automatically delegates a user's votes to themselves if a delegatee is not set.

VotesUpgradeable.sol#L165-168

function delegates(address account) public view virtual returns (address) {
    VotesStorage storage $ = _getVotesStorage();
    return $._delegatee[account] == address(0) ? account : $._delegatee[account];
}

If a user delegates their votes to the zero address, the above logic will still return that a user has delegated their votes to themselves.

This will mess up the logic in _moveDelegateVotes() VotesUpgradeable.sol#L233-L253

function _moveDelegateVotes(address from, address to, uint256 amount) private {
    VotesStorage storage $ = _getVotesStorage();
    if (from != to && amount > 0) {
        if (from != address(0)) {
            (uint256 oldValue, uint256 newValue) = _push(
                $._delegateCheckpoints[from],
                _subtract,
                SafeCast.toUint208(amount)
            );
            emit DelegateVotesChanged(from, oldValue, newValue);
        }
        if (to != address(0)) {
            (uint256 oldValue, uint256 newValue) = _push(
                $._delegateCheckpoints[to],
                _add,
                SafeCast.toUint208(amount)
            );
            emit DelegateVotesChanged(to, oldValue, newValue);
        }
    }
}

If a user tries to redelegate to themselves, _moveDelegateVotes() will receive the same address for from and to and skip the delegation.

If a user tries to delegate to another address, it will view from as having zero delegates (or not enough delegates) and will revert due to underflow.

Impact

If a user delegates their votes to the zero address, those votes will be locked forever. Additionally, that user will no longer be able to delegate their votes to another address.

This also makes misuse and phishing attacks more feasible because it involves interacting directly with the trusted contract.

Proof of Concept

function testVotesDelegatedToAddressZeroLostForever() public {
    // Mint tokens to the user
    uint256 mintAmount = 1000 * 1e18;
    vm.prank(address(erc20TokenEmitter));
    erc20Token.mint(address(this), mintAmount);
    assertEq(erc20Token.getVotes(address(this)), mintAmount);

    // Delegate voting power to address 0
    erc20Token.delegate(address(0));
    assertEq(erc20Token.getVotes(address(this)), 0);

    // Try to delegate back to user and also other address
    erc20Token.delegate(address(this));
    assertEq(erc20Token.getVotes(address(this)), 0);

    vm.expectRevert();
    erc20Token.delegate(address(10));
    assertEq(erc20Token.getVotes(address(10)), 0);
    assertEq(erc20Token.getVotes(address(this)), 0);

    // Mint more tokens to the user
    vm.prank(address(erc20TokenEmitter));
    erc20Token.mint(address(this), mintAmount);
    assertEq(erc20Token.balanceOf(address(this)), mintAmount * 2);

    // User can no longer delegate votes
    vm.expectRevert();
    erc20Token.delegate(address(10));

    // Only newly minted tokens are accounted for
    assertEq(erc20Token.getVotes(address(10)), 0);
    assertEq(erc20Token.getVotes(address(this)), mintAmount);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Check that a user does not delegate their votes to the zero address.

VotesUpgradeable.sol#L172-L175

function delegate(address delegatee) public virtual {
+   require(delegatee != address(0), "Cannot delegate to address(0)");
    address account = _msgSender();
    _delegate(account, delegatee);
}

Assessed type

Under/Overflow