Users can front-run calls to `change_gauge_weight` to gain extra voting power

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/GaugeController.sol#L204

Vulnerability details

Impact

Users can get extra voting power by front-running calls to change_gauge_weight

Proof of Concept

It can be expected that in some cases calls will be made to change_gauge_weight to increase or decrease a gauge's weight. The problem is users can be monitoring the mempool expecting such calls. Upon seeing such, any people who have voted for said gauge can just remove their vote prior to change_gauge_weight. Once it executes, they can vote again for their gauge, increasing its weight more than it was expected to be: Example:

1. Gauge has 1 user who has voted and contributed for 10_000 weight
2. They see an admin calling change_gauge_weight with value 15_000.
3. User front-runs it and removes all their weight. Gauge weight is now 0.
4. Admin function executes. Gauge weight is now 15_000
5. User votes once again for the gauge for the same initial 10_000 weight. Gauge weight is now 25_000.

Gauge weight was supposed to be changed from 10_000 to 15_000, but due to the user front-running, gauge weight is now 25_000

Tools Used

Manual review

Recommended Mitigation Steps

Instead of having a set function, use increase/ decrease methods.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #401

[c4-judge]

alcueca marked the issue as duplicate of #288

[c4-judge]

alcueca marked the issue as partial-50

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid

[c4-judge]

alcueca marked the issue as not a duplicate

[alcueca]

Votes are only counted at the first second of an epoch, front-running change_gauge_weight won't do anything.

[deadrosesxyz]

Hey, I'm leaving a simple PoC showcasing the issue and the respective logs from it:

    function testWithFrontRun() public { 
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.change_gauge_weight(gauge1, 0);
        vm.stopPrank();

        vm.startPrank(user1);
        ve.createLock{value: 1 ether}(1 ether);
        gc.vote_for_gauge_weights(gauge1, 10000);
        uint weight = gc.get_gauge_weight(gauge1);
        console.log("gauge's weight after voting: ", weight);
        vm.stopPrank();

        vm.prank(user1);
        gc.vote_for_gauge_weights(gauge1, 0); // front-run transaction by user
        vm.prank(gov);
        gc.change_gauge_weight(gauge1, 1000000000000000000);

        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, 10000); // back-run
        weight = gc.get_gauge_weight(gauge1);
        console.log("gauge's weight after changing weight: ", weight);
        vm.stopPrank();
    }

        function testWithoutFrontRun() public { 
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.change_gauge_weight(gauge1, 0);
        vm.stopPrank();

        vm.startPrank(user1);
        ve.createLock{value: 1 ether}(1 ether);
        gc.vote_for_gauge_weights(gauge1, 10000);
        uint weight = gc.get_gauge_weight(gauge1);
        console.log("gauge's weight after voting: ", weight);
        vm.stopPrank();

        vm.prank(gov);
        gc.change_gauge_weight(gauge1, 1000000000000000000);

        weight = gc.get_gauge_weight(gauge1);
        console.log("gauge's weight after government changes weight: ", weight);
    }

[PASS] testWithFrontRun() (gas: 702874)
Logs:
  gauge's weight after voting:  993424657416307200
  gauge's weight after changing weight:  1993424657416307200

[PASS] testWithoutFrontRun() (gas: 674264)
Logs:
  gauge's weight after voting:  993424657416307200
  gauge's weight after government changes weight:  1000000000000000000

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as satisfactory

[alcueca]

From the sponsor:

In practice the function should only be used to set the weights to 0 (e.g., when a market got exploited and not all users withdrew their votes yet). But there is nothing that prevents it from setting it to a higher or lower value (at least not in the audited codebase, added that in the meantime). So what is described in the finding is generally true, although it does not require frontrunning or anything like that, because the function just changes the current voting result, anyway. So if voting for the market is not restricted afterwards (by removing it from the whitelist), people can continue to vote on it and the final vote result may be different than what governance has set with this function.

[alcueca]

From what I understand the function is essentially broken, and a Medium severity is appropriate given that wardens can't assume that it will be used only to set the weights to 0 for non-whitelisted markets.

[OpenCoreCH]

Fixed in https://github.com/mkt-market/canto-verwa-protocol/pull/4