veRWA - bart1e's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L383 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L331 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L294 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L274

Vulnerability details

Note: the current delegation logic is broken anyway as I have outlined in my previous finding - the problem is that in order to delegate back to himself, user has to extend his locking period by 5 years. In this submission, I'm assuming that it is fixed, as otherwise, that bug would also manifest here and the analysis would be more complicated.

Users can stake their money and receive voting power in return (that decrease gradually over 5 years). They can also delegate their voting power to someone else. The problem is that if, for some reason, they don't or are unable to delegate back to themselves before their locking period ends, their staked money will be locked forever.

It's because the following reasons:

1. User can only withdraw his money when his current delegatee is himself.
2. In delegate, there is the following statement: require(toLocked.end > block.timestamp, "Delegatee lock expired");. If user tries to delegate back to himself, it will revert since current block.timestamp is lower than toLocked.end (toLocked is LockedBalance struct for that user).
3. The same require can be found in the increaseAmount function, so user cannot increase his locked.end there.
4. He cannot do this in createLock either, since it requires that locked_.amount == 0.
5. So user cannot withdraw and cannot increase his locking period, because all functions that could potentially do that will fail.
6. It means that toLocked.end will always remain < block.timestamp, so the functions will always revert.

So, the following scenario can happen:

1. Bob stakes some money for 5 years and so does Alice.
2. Bob delegates his voting power for Alice after some time (or immediately).
3. After 5 years:

• Bob either doesn't know that he has to delegate back to himself before his locking period end in order to withdraw (which definitely isn't obvious even for people who know Solidity and let alone for a simple user)
• Bob knows it, but still forgets to do it - 5 years is a lot of time and he won't be thinking about the protocol every day, so he can realise that he wants to withdraw after 5 years and 1 month, for example
• Bob knows that he has to delegate back to himself before that time passes and he intends to do it, but he gets injured (or something other happens to him) and he has to go to the hospital for several weeks and he is simply unable to undelegate his voting power on time

4. Bob's money will be lost.

It's important to realise that it can't be classified as a "user's mistake" since there are many situations (as I have shown above) where user is unable or unaware that he has to perform some action in order to get his money back and if he doesn't, he will lose all of his money.

Impact

Users may be unable to get back their money that they staked for 5 years. Since the impact is high, and as I explained above, it may happen due to a bunch of different reasons, I'm submitting this issue as High.

Proof of Concept

Please run the following test:

function testPoC4() public
    {
        // `user1` and `user2` lock some money
        vm.prank(user1);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);

        vm.prank(user2);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);

        // `user1` decides to delegate to `user2`
        vm.prank(user1);
        ve.delegate(user2);

        // 5 years pass
        vm.warp(block.timestamp + 1825 days);

        (, uint256 end, , ) = ve.locked(user1);
        assertLe(end, block.timestamp);

        vm.startPrank(user1);        
        // `user1` won't be able to withdraw his money, after his lock period came to an end
        vm.expectRevert("Lock delegated");
        ve.withdraw();

        // `user1` can't delegate back to himself either
        vm.expectRevert("Delegatee lock expired");
        ve.delegate(user1);

        // he cannot even increase his locking amount in order to increase locking period
        vm.expectRevert("Lock expired");
        ve.increaseAmount{value: 1}(1);
    }

Tools Used

VS Code

Recommended Mitigation Steps

Allow users to delegate to themselves even when their locking period came to an end (and, if current delegatee's locked.end is greater, then increase locked.end of the user who wants to delegate to himself to delegatee's locked.end as suggested in my previous submission).

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L91-L114 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L142 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L180 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L189 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L247

Vulnerability details

_get_weight function is used in order to return the total gauge's weight and it also updates past values of the points_weight mapping, if time_weight[_gauge_addr] is less or equal to the block.timestamp. It contains the following loop:

            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;
            }

There are two possible scenarios:

• pt.bias > d_bias
• pt.bias <= d_bias

The first scenario will always happen naturally, since pt.bias will be the total voting power allocated for some point and since slope is a sum of all users' slopes and slopes are calculated in such a way that <SLOPE> * <TIME_TO_END_OF_STAKING_PERIOD> = <INITIAL_BIAS>.

However, it is possible to artificially change points_weight[_gauge_addr][t].bias by calling change_gauge_weight (which can be only called by the governance). It important to notice here, that change_gauge_weight doesn't modify points_weight[_gauge_addr][t].slope

change_gauge_weight does permit to change the weight to a smaller number than its current value, so it's both perfectly legal and possible that governance does this at some point (it could be changing the weight to 0 or any other value smaller than the current one).

Then, at some point when _get_weight is called, we will enter the else block because pt.bias will be less than the sum of all user's biases (since originally these values were equal, but pt.bias was lowered by the governance). It will set pt.bias and pt.slope to 0.

After some time, the governance may realise that the gauge's weight is 0, but should be bigger and may change it to some bigger value.

We will have the situation where points_weight[_gauge_addr][t].slope = 0 and points_weight[_gauge_addr][t].bias > 0.

If this happens and there is any nonzero changes_weight[_gauge_addr] not yet taken into account (for instance in the week after the governance update), then all the functions related to the gauge at _gauge_addr will not work.

It's because, the following functions:

• checkpoint_gauge
• gauge_relative_weight_write
• gauge_relative_weight
• _change_gauge_weight
• change_gauge_weight
• vote_for_gauge_weights
• remove_gauge

call _get_weight at some point.

Let's see what will happen in _get_weight when it's called:

                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 {

We will enter the if statement, because pt.bias will be > 0 and pt.slope will be 0 (or some small value, if users give their voting power to gauge in the meantime), since it was previously set to 0 in the else statement and wasn't touched when gauge's weight was changed by the governance. We will:

• subtract d_bias from pt.bias which will succeed
• attempt to subtract changes_weight[_gauge_addr][t] from d_slope

However, there could be a user (or users) whose voting power allocation finishes at t for some t not yet handled. It means that changes_weight[_gauge_addr][t] > 0 (and if pt.slope is not 0, then changes_weight[_gauge_addr][t] still may be greater than it).

If this happens, then the integer underflow will happen in pt.slope -= d_slope;. It will now happen in every call to _get_weight and it won't be possible to recover, because:

• vote_for_gauge_weights will revert
• change_gauge_weight will revert

as they call _get_weight internally. So, it won't be possible to modify pt.slope and pt.bias for any point in time, so the revert will always happen for that gauge. It won't even be possible to remove that gauge.

So, in short, the scenario is as follows:

1. Users allocate their voting power to a gauge X.
2. Governance at some point decreases the weight of X.
3. Users withdraw their voting power as the time passes, and finally the weight of X drops to 0.
4. Governance realises this and increases weight of X since it wants to incentivise users to provide liquidity in X.
5. Voting power delegation of some user(s) ends some time after that and _get_weight attempts to subtract changes_weight[_gauge_addr][t] from the current slope (which is either 0 or some small value) and it results in integer underflow.
6. X is unusable and it's impossible to withdraw voting power from (so users cannot give their voting power somewhere else). The weight of X cannot be changed anymore and X cannot be even removed.

Note that it is also possible to frontrun the call to change_gauge_weight when the weight is set to a lower value - user with a lot of capital can watch the mempool and if weight is lowered to some value x, he can give a voting power of x to that gauge. Then, right after weight is changed by the governance, he can withdraw his voting power, leaving the gauge with weight = 0. Then, governance will manually increase the weight to recover and DoS will happen as described. So it is only needed that governance decreases gauge's weight at some point.

Impact

As stated, above the impact is that the entire gauge is useless, voting powers are permanently locked there and its weight is impossible to change, so the impact is high.

In order for this situation to succeed, governance has to decrease weight of some gauge, but I think it's very likely, because:

1. _get_weight checks that if (pt.bias > d_bias) and it handles the opposite situation, so it is anticipated that it may genuinely happen.
2. It is definitely possible to decrease gauge's weight and it's even possible to zero it out (as in the remove_gauge).
3. The situation where old_bias is greater than old_sum_bias + new_bias is handled in vote_for_gauge_weights, but it may only happen when gauge's weight was decreased by the governance.
4. The situation where old_slope.slope is greater than old_sum_slope + new_slope.slope is also handled there, but it may only happen if we enter the else statement in _get_weight.

So, it is predicted that gauge's weight may be lowered and the protocol does its best to handle it properly, but as I showed, it fails to do so. Hence, I believe that this finding is of High severity, because although it requires governance to perform some action (decrease weight of some gauge), I believe that it's likely that governance decides to decrease weight, especially that it is anticipated in the code and edge cases are handled there (and they wouldn't be if we assumed that governance would never allowed them to happen).

Proof of Concept

Please run the test below. The test shows slightly simplified situation where governance just sets weight to 0 for gauge1, but as I've described above, it suffices that it's just changed to a smaller value and it may drop to 0 naturally as users withdraw their voting power. The following import will also have to be added: import {Test, stdError} from "forge-std/Test.sol";.

function testPoC1() public
    {
        // gauge is being set up
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.change_gauge_weight(gauge1, 0);
        vm.stopPrank();

        // `user1` pays some money and adds his power to `gauge1`
        vm.startPrank(user1);
        ve.createLock{value: 1 ether}(1 ether);
        gc.vote_for_gauge_weights(gauge1, 10000);
        vm.warp(block.timestamp + 10 weeks);
        gc.checkpoint_gauge(gauge1);
        vm.stopPrank();

        // `user2` does the same
        vm.startPrank(user2);
        ve.createLock{value: 1 ether}(1 ether);
        gc.vote_for_gauge_weights(gauge1, 10000);
        vm.warp(block.timestamp + 1 weeks);
        gc.checkpoint_gauge(gauge1);
        vm.stopPrank();

        vm.warp(block.timestamp + 1825 days - 14 weeks);
        vm.startPrank(gov);
        // weight is changed to `0`, just to simplify
        // normally, weight would just be decreased here and then subsequently decreased by users when their
        // locking period is over until it finally drops to `0`
        // alternatively, some whale can frontrun a call to `change_gauge_weight` as described and then
        // withdraw his voting power leaving the gauge with `0` slope and `0` bias
        gc.change_gauge_weight(gauge1, 0);
        vm.warp(block.timestamp + 1 weeks);
        
        // now, weight is changed to some bigger value
        gc.change_gauge_weight(gauge1, 1 ether);
        vm.stopPrank();
        // some time passes so that user1's locking period ends
        vm.warp(block.timestamp + 5 weeks);
        
        // `user2` cannot change his weight although his `locked.end` is big enough
        vm.prank(user2);
        vm.expectRevert(stdError.arithmeticError);
        gc.vote_for_gauge_weights(gauge1, 0);

        // governance cannot change weight
        vm.startPrank(gov);
        vm.expectRevert(stdError.arithmeticError);
        gc.change_gauge_weight(gauge1, 2 ether);
        
        // governance cannot even remove the gauge
        // it's now impossible to do anything on gauge1
        vm.expectRevert(stdError.arithmeticError);
        gc.remove_gauge(gauge1);
        vm.stopPrank();
    }

Tools Used

VS Code

Recommended Mitigation Steps

Perform pt.slope -= d_slope in _get_weight only when pt.slope >= d.slope and otherwise zero it out.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L331 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L384 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L301

Vulnerability details

In the VotingEscrow contract users can lock their money for 5 years in order to get some voting power (that will gradually decrease over these 5 years). It is also possible to delegate own voting power to someone else by calling delegate.

However, due to the require(toLocked.end >= fromLocked.end, "Only delegate to longer lock"); in that function, in order to delegate back to himself, the user has to have locked.end greater than the locked.end for the current delegatee.

Consider the following scenario:

1. Bob stakes some money.
2. Alice stakes some money several weeks after Bob.
3. Bob decides to delegate his voting power to Alice.
4. After almost 5 years, Bob sees that his locking period is finally coming to an end, and he knows that in order to withdraw, he has to be his current delegatee, so he calls delegate(Bob).
5. That call will fail, since Bob's locking period end is less than Alice's.
6. The only way to recover is to extend Bob's locking period and then to call delegate to himself, but extending locking period is only possible by calling increaseAmount, which will extend Bob's locking period by 5 years.
7. So, if Bob wants to withdraw his money, he has to stake for 5 years more, which is a lot in a real world, let alone in the crypto world.

Notice that it wasn't a Bob's mistake - he just delegated to somebody and out of the sudden, the protocol forced him to lock his money for the next 5 years. It's definitely not clear to users that protocol behaves in this way.

Impact

Users will have to stake money for up to 5 years more than intended, which is unacceptable as 5 years is a lot of time.

The only condition that has to happen is that user delegates his voting power to someone else, which definitely is not only permitted behaviour, but it's also expected.

Since the impact is high and the probability is also high, I'm submitting this issue as High.

Proof of Concept

Please run the following test:

function testPoC3() public
    {
        // `user` and `user2` lock some money
        vm.prank(user1);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);

        vm.warp(block.timestamp + 10 * WEEK);

        vm.prank(user2);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);

        // `user1` decides to delegate to `user2`
        vm.prank(user1);
        ve.delegate(user2);

        // almost 5 years passes
        vm.warp(block.timestamp + 1825 days - 11 * WEEK);
        
        // `user1` sees that his locking period is coming to and end, so he wants to delegate back to himself
        // so that he will be able to withdraw in the next couple of weeks; but he will fail
        vm.startPrank(user1);
        vm.expectRevert("Only delegate to longer lock");
        ve.delegate(user1);

        vm.warp(block.timestamp + 4 * WEEK);

        (, uint256 end, , ) = ve.locked(user1);
        assertLe(end, block.timestamp);
        
        // `user1` won't be able to withdraw his money either, when his lock period comes to an end
        // he has to stake for 5 more years in order to recover
        vm.expectRevert("Lock delegated");
        ve.withdraw();
    }

Tools Used

VS Code

Recommended Mitigation Steps

delegate should handle differently the delegations from the user to himself.

While the restriction that the new delegatee should have higher (or equal) locked.end than the current delegatee has a lot of sense (in order to avoid some abuses), it should be imposed differently when user wants to delegate back to himself.

In such a case, instead of reverting when his locked.end is smaller than his current delegatee's locked.end, his locked.end should be set to the delegatee's locked.end. It will increase his locking period, but it's necessary in order to avoid the situation where we allow to delegate to someone with shorter locking period left.

However, locking period increase won't normally be drastic (in the example I have shown, Bob will have to just wait a couple of weeks more, which is acceptable, instead of 5 years) and as long as it's communicated to the users that their locking period will increase to the delegatee's one once they delegate, the protocol's behaviour will be correct.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L331 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L384

Vulnerability details

Note: current implementation of delegate is buggy as I have described in one of my previous submissions. The correct implementation would increase locked.end of user to his new delegatee's locked.end when he delegates. I'm assuming that it works correctly, because otherwise, this bug would also manifest here and the analysis would be more complicated and less understandable.

If user A delegates to the user B and at some point of time he wants to withdraw his money, he has to delegate back to himself. In order to do so, the following inequality has to hold: toLocked.end >= fromLocked.end. If we assume that toLocked (which is the LockedBalance for A) was updated (if toLocked wasn't updated, the situation would be identical) such that toLocked == fromLocked.end, then still fromLocked.end may be increased by delegatee at any time by calling increaseAmount.

It means that B's locked.end is greater than A's locked.end + <SEVERAL_YEARS> and it was out of A's control. It may happen because of the two reasons:

• B frontruns A and if he sees that A tries to undelegate from him (no matter where), he may frontrun A and extend his locking period, which will cause A's call to delegate to revert
• B just extends his locking period at some point throughout 5 year staking period and doesn't want to do anything malicious to A

Regardless of the reason, if it happens, then the only way for A to get his money back, is to extend his locking period by 5 years in order to call delegate successfully. 5 years is a lot and user's staking period shouldn't be dependent on actions performed by other users.

Impact

User may be forced to stake for up to 5 more years which is a lot of time, so the impact is high. The only reason for submitting this issue as Medium is that I assume that users will delegate only to trusted users (so that they will not perform the frontrunning attack) and that these trusted users will not increase their locking time (but this requires them to know about the issue I'm submitting, so this finding may be classified as High as well - I'm leaving this decision to the Judge and to Sponsors).

Proof of Concept

Please run the following test:

function testPoC5() public
    {
        // `user1` and `user2` lock some money
        vm.prank(user1);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);

        vm.prank(user2);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);

        // `user1` decides to delegate to `user2`
        vm.startPrank(user1);
        ve.delegate(user2);

        // check that user can delegate back to himself successfully
        ve.delegate(user1);

        // we bring back the previous state when `user1` delegated to `user2`
        ve.delegate(user2);
        vm.stopPrank();

        // several years pass
        vm.warp(block.timestamp + 1500 days);

        // `user2` decides to increase his locking period for the next 5 years
        vm.prank(user2);
        ve.increaseAmount{value: 1}(1);

        // `user1` has to call `increaseAmount` which will force him to stake for several years longer than he wanted
        vm.prank(user1);
        vm.expectRevert("Only delegate to longer lock");
        ve.delegate(user1);
    }

Tools Used

VS Code

Recommended Mitigation Steps

Do one of the following:

• acknowledge the issue and clearly inform users that such things may happen (although it will make users reluctant to delegate at all)
• when user delegates to someone, increase his locking period accordingly and save delegatee's locked.end. Then, in delegate, instead of comparing toLocked.end with fromLocked.end, compare toLocked.end with a saved value which reflects the fromLocked.end but at the time the delegation was made. It will still prevent from delegating to someone with shorter locking period left, but it will also mitigate the issue when delegatee increases his locking period

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L127-L132 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/GaugeController.sol#L211-L213

Vulnerability details

Governance can use the GaugeController::remove_gauge function in order to mark some gauge as invalid and set its weight to 0. I assume that it is going to be used in some emergency situations when a gauge is hacked, for example, so that it doesn't receive more CANTO allocation. However, the only correct use of this function is when there is noone currently allocating his voting power for a gauge.

In order to see this, imagine a situation where:

1. Bob allocates 50% (5000 / 10000) of his voting power to a gauge X.
2. Governance decides to remove X because of some issues with X, so it calls remove_gauge.
3. It will do two things: set X's weight to 0 (which is totally OK) and set isValidGauge[_gauge] to false.
4. Bob will notice that there is no incentive for him to allocate his power anymore for that gauge and will try to call vote_for_gauge_weights with _user_weight = 0. He will need to do this, since there is require(power_used >= 0 && power_used <= 10_000) later on in that function, so if he doesn't withdraw his voting power, he won't be able to dedicate more than 50% of his voting power to other gauges.
5. However, vote_for_gauge_weights will revert since it requires that isValidGauge[_gauge_addr] is true.

While it may seem a desired behaviour, it shouldn't work this way as it blocks users from performing an important operation. If gauge is not going to be used anymore, then change_gauge_weight with _weight = 0 may be called and the gauge will not receive any CANTO anyway, but users will have a possibility to withdraw their voting power allocation. remove_gauge should be called only when everybody withdraws his voting power from the gauge and it should only set isValidGauge[_gauge] to false.

So, change_gauge_weight to 0 will give the same end effect, but will also give users a possibility to withdraw their voting power from the gauge. Moreover, if we assume that users know what they are doing (and that is the assumption from the contest README), they will not allocate their voting power to a gauge with weight 0, so removing gauge doesn't benefit the protocol at all and may be harmful for the users.

Impact

The impact is that user's voting power may be locked in a gauge for a long time, until governance decides to artificially change isValidGauge to true for some gauge, so that users can remove their voting power from the gauge. If user's voting power is locked, they cannot allocate it somewhere else, so they cannot increase CANTO allocation for some other gauge, where they could receive more money.

Since money gain opportunity can be lost and users won't be able to use some protocol functionality, but it requires governance to remove some gauge, I'm submitting this issue as Medium.

Proof of Concept

Please run the following test and add the following import: import {Test, stdError} from "forge-std/Test.sol";:

function testPoC2() public
    {
        // gauge is being set up
        vm.startPrank(gov);
        gc.add_gauge(gauge1);
        gc.change_gauge_weight(gauge1, 0);
        vm.stopPrank();

        // `user1` pays some money and adds his power to `gauge1`
        vm.startPrank(user1);
        ve.createLock{value: 1 ether}(1 ether);
        gc.vote_for_gauge_weights(gauge1, 10000);
        vm.warp(block.timestamp + 1 weeks);
        gc.checkpoint_gauge(gauge1);
        vm.stopPrank();

        // governance removes gauge
        vm.prank(gov);
        gc.remove_gauge(gauge1);

        // `user1` won't be able to remove his power commitment
        vm.prank(user1);
        vm.expectRevert("Invalid gauge address");
        gc.vote_for_gauge_weights(gauge1, 0);
    }

Tools Used

VS Code

Recommended Mitigation Steps

remove_gauge should either be removed entirely, or it should only be called when noone is allocating his voting power to a gauge after changing the gauge weight to 0 previously (or at least when some time period, like 1 year, passes from setting the gauge weight, so that users have enough time to react).

Assessed type

Other