Users delegating veCANTO may be forced into locking funds for more than the 5 years anticipated

[code423n4]

Lines of code

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

Vulnerability details

VotingEscrow has one check in place for avoiding vote manipulation: delegation can move funds only from a shorter to a longer-locking account, and this includes when the delegation is undone. This is the root cause of an issue that is below.

This presents the scenario of Alice locking her 1M CANTO in VotingEscrow, and delegating to Bob because she is not very into keeping an eye on the votings happening.

Bob, on the contrary, is likely to be an entity who uses VotingEscrow just for receiving delegations of many users and voting on their behalf.
Both of these motivations offer Bob a concrete incentive in keeping their lock as far in time as possible (i.e. keeping it at 5 years by increasing their funds by 1 wei per week), because:

• if Bob does not shift their lock far in time, it will not be entitled to receive new delegations of freshly-locked funds (because funds can be delegated only to accounts locked for longer, and freshly-locked funds can delegate only to accounts that have in turn very recently refreshed their lock). On the contrary, increasing their funds by 1 wei per week, will enable anybody to delegate to them
• if Bob does not shift their lock far in time, their voting power will shrink over time, and since Bob's purpose may be just about voting, Bob has all the interest in retaining more power

When Bob acts this way (which, as said, seems the most reasonable way to act for Bob), Alice may:

• delegate 1M CANTO to Bob today
• set an alarm on removing delegation in just before 5 years
• at that point, just before the 5 years, Alice tries to remove the delegation
• this action fails, because when un-delegating, funds have to flow to a longer-locked account

Impact

To withdraw the funds, Alice has only one option:

• add 1 wei of veCanto,
• un-delegate her 1M CANTO,
• and wait 5 years without delegating

Proof of Concept

The situation can be showed in the following Foundry test:

    function testWithdrawAfterDelegation() public {
        uint256 currentTs = block.timestamp;

        vm.prank(alice);
        ve.createLock{value: 10 ether}(10 ether);

        vm.prank(bob);
        ve.createLock{value: 10 wei}(10 wei);

        vm.prank(alice);
        ve.delegate(bob);

        vm.warp(currentTs + LOCKTIME - 10 days);
        vm.prank(bob);
        ve.increaseAmount{value: 10 wei}(10 wei);

        // Alice can't withdraw
        vm.startPrank(alice);
        vm.expectRevert("Only delegate to longer lock");
        ve.delegate(alice);

        // The only option to get the CANTO back is to lock for 5 more years
        ve.increaseAmount{value: 10 wei}(10 wei);
        ve.delegate(alice);

        vm.warp(currentTs + 2 * LOCKTIME - 20 days);
        ve.withdraw();
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

There is, unfortunately, no easy fix I can think of to this issue because simply removing the "Only delegate to longer lock" check, even only for un-delegating, can easily open the possibility for bypassing the desired vote power decay over time.

Maybe it could be worth considering a change in the design:

• restore the original library's use of "balance" and remove the concept of "delegate balance"
• maintain instead a map from delegatee to list of delegators
• whenever the voting power of a user is calculated, do the calculation based on their own curve, and iteratively add the results of similar calculations done with the curves of each of their delegators

Doing so will make the voting power of each delegator decay according to the original (delegator's) lock, so funds can be withdrawn when the original lock expires without opening the possibility of voting manipulation.

Assessed type

Governance

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as duplicate of #82

[c4-pre-sort]

141345 marked the issue as not a duplicate

[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 duplicate of #82

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as satisfactory

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as duplicate of #375

[c4-judge]

alcueca marked the issue as duplicate of #182

[c4-judge]

alcueca changed the severity to 3 (High Risk)