Delegate system can be used to increase voting power without resetting the lockEnd

[code423n4]

Lines of code

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

Vulnerability details

Impact

The added delegate mechanism allows increasing the voting power without resetting the lockEnd.

Proof of Concept

IncreaseAmount() increases the locked amount of a msg.sender staked native in VotingEscrow and resets its lockEnd to block.timestamp + LOCK_TIME. Users can bypass this feature by generating several wallets, creating locks for these wallets and delegating to the first lock. There is a check in the code that requires that a lock can only be delegated to a more recent lock, but it can be bypassed by delegating in the same block that the first lock is created.

A POC was carried out in Foundry, add the following test to VotingEscrow.t.sol:

function testIncreaseVotingPowerWithoutResettingLockEnd() public {
    vm.deal(user1, 100 ether);

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

    // generate some wallets to delegate in the future if needed to increase
    // voting power without resetting the lock end
    for (uint256 i_; i_ < 10; i_++) {
        address fakeUser1_ = vm.addr(i_ + 27); // random number
        vm.deal(fakeUser1_, 100 ether);
        vm.startPrank(fakeUser1_);
        ve.createLock{value: 1}(1); // just create the lock
        ve.delegate(user1);
        vm.stopPrank();
    }

    (int128 amount_, uint256 lockEnd_,,) = ve.locked(user1);

    // 100 days later, user wants to increase voting power without resetting the lock end
    vm.warp(block.timestamp + 100 days);

    vm.prank(vm.addr(27)); // one of the fake users
    ve.increaseAmount{value: 1 ether}(1 ether);

    (int128 finalAmount_, uint256 finalLockEnd_, int128 delegated_, ) = ve.locked(user1);

    // amount remains equal
    assertEq(amount_, finalAmount_);

    // same lock end, but as can be seen below, the voting power will increase
    assertEq(lockEnd_, finalLockEnd_);

    // 1e18 initial amount + 1e18 increase amount from one of the fake delegates
    assertEq(delegated_, 2e18 + 10);
}

As can be seen, the user is able to increase their voting power without increasing the lockEnd of the original stake.

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

The current requirement that a lock must be delegated to a longer lock is the following:

require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

Change the requirement to be strictly bigger as following:

require(toLocked.end > fromLocked.end, "Only delegate to longer lock");

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #45

[c4-pre-sort]

141345 marked the issue as duplicate of #99

[c4-pre-sort]

141345 marked the issue as duplicate of #178

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as low quality report

[141345]

invalid

delegator/owner increaseAmount(), delegator's locked.end extended, original delegatee's locked.end stays the same, vice versa

[alcueca]

Seems valid to me, @OpenCoreCH?

[OpenCoreCH]

Took me some time, but this is intended behaviour and the delegation does not make a difference here:

In the PoC, vm.addr(27) delegates to user1. The lock of user1 is not increased, but the one of vm.addr(27) is, which can be checked by changing two lines in the PoC:

(int128 amount_, uint256 lockEnd_, ,) = ve.locked(vm.addr(27));

(int128 finalAmount_, uint256 finalLockEnd_, int128 delegated_, ) = ve.locked(vm.addr(27));

So we have:

• The attacker has a voting power of ~2e18
• He can withdraw 1e18 after 5 years (the amount that belongs to user1) and the other 1e18 after 5 years and 100 days (the amount that belongs to vm.addr(27))

Compare that to a situation where the attacker never delegates and just uses two wallets to have different end timestamps. We also assume that in the second wallet, the amount is increased after 100 days. This results in exactly the same situation:

• The attacker has a voting power of ~2e18
• He can withdraw 1e18 after 5 years (the amount in the first wallet) and the other 1e18 after 5 years and 100 days (the amount in the second wallet)

The delegation does not make a difference here and the end timestamp of the user that increases the amount is correctly increased, whereas the one of the delegatee stays constant.

[c4-judge]

alcueca marked the issue as unsatisfactory:
Invalid

[Simao123133]

@OpenCoreCH comment is very good and It crossed my mind just after submitting the issue. However, I think there is a small caveat, it's not the same having all the voting power in a wallet or split accross different wallets.

As shown in the test below, having the power concentrated in 1 user seems to give more voting power than 2 users with half locked each, being advantageous to perform this attack.

function test_POC_OneWalletVsTwoWallets() public {
    vm.deal(user1, 100 ether);
    vm.startPrank(gov);
    gc.add_gauge(user1);
    gc.change_gauge_weight(user1, 100);
    vm.stopPrank();

    uint256 v = 10 ether;

    vm.startPrank(user1);
    ve.createLock{value: 2*v}(2*v);
    gc.vote_for_gauge_weights(user1, 10_000);
    vm.stopPrank();

    /*vm.startPrank(user2);
    ve.createLock{value: v}(v);
    gc.vote_for_gauge_weights(user1, 10_000);
    vm.stopPrank();*/

    uint256 expectedWeight_ = gc.get_gauge_weight(user1);

    console.log("expected", expectedWeight_);


    // 19868493150519148900 2 users
    // 19868493150675792100 1 user
}

There might be other differences besides this one.
@alcueca kindly ask you to review the issue with this in mind

[alcueca]

As shown in the test, there is a loss of voting power of 10^-9% by splitting voting power between two accounts. That would be about expected if there is any rounding in the system.

[Simao123133]

I can't find any benefit other than this by having all the power in only one account. I'll trust your judgement. Thank you!