GaugeController allows for quick vote and withdraw voting strategy

[code423n4]

Lines of code

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

Vulnerability details

Impact

The GaugeController voting can be abused to apply all of the user’s weight in every gauge’s vote. GaugeController’s voting changes the weight of the gauges. Each user can split their voting weight power between the gauges. The sum of all the weight used must not exceed 10,000 (this is ensured).

function vote_for_gauge_weights(address _gauge_addr, uint256 _user_weight) external {
    require(_user_weight >= 0 && _user_weight <= 10_000, "Invalid user weight");
    require(isValidGauge[_gauge_addr], "Invalid gauge address");
    VotingEscrow ve = votingEscrow;
    (
        ,
        /*int128 bias*/
        int128 slope_, /*uint256 ts*/

    ) = ve.getLastUserPoint(msg.sender);
    require(slope_ >= 0, "Invalid slope");
    uint256 slope = uint256(uint128(slope_));
    uint256 lock_end = ve.lockEnd(msg.sender);
    uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;
    require(lock_end > next_time, "Lock expires too soon");
    VotedSlope memory old_slope = vote_user_slopes[msg.sender][_gauge_addr];
    uint256 old_dt = 0;
    if (old_slope.end > next_time) old_dt = old_slope.end - next_time;
    uint256 old_bias = old_slope.slope * old_dt;
    VotedSlope memory new_slope = VotedSlope({
        slope: (slope * _user_weight) / 10_000,
        end: lock_end,
        power: _user_weight
    });
    uint256 new_dt = lock_end - next_time;
    uint256 new_bias = new_slope.slope * new_dt;

    // Check and update powers (weights) used
    uint256 power_used = vote_user_power[msg.sender];
    power_used = power_used + new_slope.power - old_slope.power;
    require(power_used >= 0 && power_used <= 10_000, "Used too much power");
    vote_user_power[msg.sender] = power_used;

    // Remove old and schedule new slope changes
    // Remove slope changes for old slopes
    // Schedule recording of initial slope for next_time
    uint256 old_weight_bias = _get_weight(_gauge_addr);
    uint256 old_weight_slope = points_weight[_gauge_addr][next_time].slope;
    uint256 old_sum_bias = _get_sum();
    uint256 old_sum_slope = points_sum[next_time].slope;

    points_weight[_gauge_addr][next_time].bias = Math.max(old_weight_bias + new_bias, old_bias) - old_bias;
    points_sum[next_time].bias = Math.max(old_sum_bias + new_bias, old_bias) - old_bias;
    if (old_slope.end > next_time) {
        points_weight[_gauge_addr][next_time].slope =
            Math.max(old_weight_slope + new_slope.slope, old_slope.slope) -
            old_slope.slope;
        points_sum[next_time].slope = Math.max(old_sum_slope + new_slope.slope, old_slope.slope) - old_slope.slope;
    } else {
        points_weight[_gauge_addr][next_time].slope += new_slope.slope;
        points_sum[next_time].slope += new_slope.slope;
    }
    if (old_slope.end > block.timestamp) {
        // Cancel old slope changes if they still didn't happen
        changes_weight[_gauge_addr][old_slope.end] -= old_slope.slope;
        changes_sum[old_slope.end] -= old_slope.slope;
    }
    // Add slope changes for new slopes
    changes_weight[_gauge_addr][new_slope.end] += new_slope.slope;
    changes_sum[new_slope.end] += new_slope.slope;

    _get_sum();

    vote_user_slopes[msg.sender][_gauge_addr] = new_slope;

    // Record last action time
    last_user_vote[msg.sender][_gauge_addr] = block.timestamp;
}

However, there is no incentive to vote early, and no lock to prevent a user from removing their weight after a vote. As a result, an attacker can put 100% of its voting power (10,000) on a gauge’s vote, and remove it right afterwards to re-use all its voting power on another vote.

Proof of Concept

1. Eve calls vote_for_gauge_weights with a voting power of 10,000 (100%) just before the vote ends.

2. If the voting ends soon after Eve votes, her weight will have a significant effect for a very short duration. Note that there's a stipulation in the code that the lock end (lock_end) should be greater than next_time (which is roughly the current time rounded to the end of the week). This means Eve should have her tokens locked for a bit longer than a week to even vote.

3. Once the vote ends, Eve calls vote_for_gauge_weights with a voting power of 0.

4. In this case, Eve is essentially withdrawing her previously applied voting power. This may reduce the overall influence of her votes, but she's trying to withdraw after the fact, once the vote ends.

The function "vote_for_gauge_weights" checks that the provided _user_weight is in the range [0, 10,000]. This is done via the require statement at the beginning of the function. Each user can split their total power among the gauges. This is reflected in the calculations (slope * _user_weight) / 10_000 which scales the voting power proportionally. There is a mechanism that ensures users don't exceed their allocated voting power, require(power_used >= 0 && power_used <= 10_000, "Used too much power").

However, there's a mechanism to cancel out the effects of old votes. If old_slope.end > block.timestamp, it deducts the old slope values from the changes.

When Eve votes with power 0 after the voting period ends, it might reset her influence on that specific gauge but the results of the voting period would already have been decided by the previous weights.

Tools Used

Manual review + in-house tool

Recommended Mitigation Steps

Voting is always tricky in such scenarios. Maybe two possible solutions work:

1. Implementing a weighted stake, with weight decreasing over time, or
2. Implementing a locking period after the weight update.

Notes

Aragon vote shows the perils of on-chain governance

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #45

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as primary issue

[141345]

withdraw() needs to go through locking period.

This one does not specifically indicate the delegate() abuse like #86

[c4-sponsor]

OpenCoreCH marked the issue as sponsor acknowledged

[c4-sponsor]

OpenCoreCH marked the issue as disagree with severity

[OpenCoreCH]

True, but I fail to see why this is a problem in our setup. The only thing that matters is the vote at the epoch end (which is queried within LendingLedger.claim). So yes, if you want to change your vote every few seconds, you can do this, but you will just waste gas and the only thing that influences the distribution is the vote at one specific point in time.

[alcueca]

Agree with the sponsor. If the warden would like to suggest a different design for the voting system, that would belong in an analysis.

[c4-judge]

alcueca marked the issue as unsatisfactory:
Invalid

[c4-judge]

alcueca marked issue #45 as primary and marked this issue as a duplicate of 45

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

From sponsor:

The problem is that no one really points out the impact this has on our usage of the GaugeController. I went through all duplicates and they either summarise the ToB finding (from here: https://github.com/trailofbits/publications/blob/master/reviews/CurveDAO.pdf) or say that GaugeController.vy has this check and we do not and that users therefore can vote and withdraw a vote directly afterwards. This is not a vulnerability per se in our usage because we always measure the votes at one specific point in time. It would be really bad if you could vote two-times with 100% for this point in time, but I have not seen any claims for that. But I will look into the issue in more detail again and see if this could maybe still lead to some unintended consequences.

I wrote a small PoC to test this behaviour in the most extreme cases:

    function testVoteGaugeWeightChangeVote() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.add_gauge(gauge2);
        vm.stopPrank();

        vm.startPrank(user1);
        ve.createLock{value: 1 ether}(1 ether);
        uint nextEpoch = (block.timestamp + WEEK) / WEEK * WEEK;
        gc.vote_for_gauge_weights(gauge1, 10000); // vote 100% for gauge1
        vm.warp(nextEpoch - 8);
        gc.vote_for_gauge_weights(gauge1, 0); // remove vote for gauge1
        gc.vote_for_gauge_weights(gauge2, 10000); // vote 100% for gauge2
        console.logUint(gc.gauge_relative_weight_write(gauge1, nextEpoch));
        console.logUint(gc.gauge_relative_weight_write(gauge2, nextEpoch));
    }

Also changed the vm.warp call to a few different values (nextEpoch, nextEpoch - 1, nextEpoch + 1). The behaviour is:
Whenever you change your vote allocation before the next epoch starts (up to nextEpoch - 1, this is reflected in the query for the next epoch.
Whenever you change your vote allocation after the next epoch has started (nextEpoch and greater), this is not reflected in the query for the next epoch and will only be reflected in the query for nextEpoch + WEEK (i.e., the epoch after nextEpoch)

[JeffCX]

this report is open and has selected for report tag, based on this comments above seems like this report can be closed with unsatisfactory tag is correctly applied to this report

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as unsatisfactory:
Invalid

[alcueca]

The selected for report tag has now been removed, thanks.