Delegators can delegate to a expired or withdrawn lock

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/VotingEscrow.sol#L356
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#L337

Vulnerability details

Impact

Users can delegate to an expired or withdrawn lock, making their voting power wasted.

Proof of Concept

In delegate(), we ensure the toLocked.end >= fromLocked.end, preventing a user to delegate on a lock that will expire before him:

    function delegate(address _addr) external nonReentrant {
        //....
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

However, in increaseAmount(), we increase the newLocked.end by 5 years and directly update the msg.sender's lock, not checking whether it currently delegating on another lock:

    function increaseAmount(uint256 _value) external payable nonReentrant {
        // ....
        newLocked.amount += int128(int256(_value));
        newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);
        if (delegatee == msg.sender) {
        //....
        } else {
            // Delegated lock, update sender's lock first
            locked[msg.sender] = newLocked; // <-----

So delegators can delegate to an expired lock:

1. user2.end > user1.end and user1 delegate to user2.
2. user1 increases his locked value, and user1.end += 5 years, user1.end > user2.end
3. When block.timestamp > user2.end, user2's lock is expired, but user1 still delegates on user2.

Also, in withdraw(), we allow there are delegated power left:

    function withdraw() external nonReentrant {
        //....
        newLocked.end = 0;
        newLocked.delegated -= int128(int256(amountToSend)); // <----

So users can delegate to a withdrawn lock in this situation.

PoC:

    // src/test/VotingEscrow.t.sol
    function testDelegateExpiredLock() public {
        // withdraw for delegated lock
        testSuccessDelegate();
        
        (, uint256 end, , ) = ve.locked(user1);
        vm.warp(end - WEEK - 1);
        vm.prank(user1);
        ve.increaseAmount{value: LOCK_AMT}(LOCK_AMT);

        assert(ve.lockEnd(user1) > ve.lockEnd(user2)); // now user1.end > user2.end even if user1 delegates to user2

        vm.warp(end + 1); // now user2 is expired, but not user1
        vm.prank(user2);
        ve.withdraw(); // user2 can withdraw even there are delegator on it

        uint256 user2_voting_power = ve.balanceOf(user2);
        console.log("user2_voting_power after withdrawl = %s\n", user2_voting_power); // Zero Voting Power
    }

Output:

qiuhao@pc:~/web3/c4/2023-08-verwa$ forge test --match-test testDelegateExpiredLock
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for src/test/VotingEscrow.t.sol:VotingEscrowTest
[PASS] testDelegateExpiredLock() (gas: 19208221)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.82ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

increaseAmount() shouldn't update the sender's lock.end bigger than delegatee's lock end.

Assessed type

Governance

[141345]

it's user's own behavior, leads to voting power waste.

QA might be more appropriate.

[OpenCoreCH]

This seems to be a duplicate of #112

[c4-sponsor]

OpenCoreCH marked the issue as sponsor confirmed

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as duplicate of #268

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as not a duplicate

[alcueca]

This is not a duplicate of #112 or #268. This finding is not about the user unable to get his CANTO back. This finding is about delegates being able to have their lock end and withdraw. In this situation, the delegators are delegating to accounts that (the warden assumes) cannot vote.

There is no mention of the delegator lock expiring, so I'm assuming that in this situation the delegator can undelegate normally. In this case the impact would be that between the delegate withdrawing and the delegator taking action, the delegator's voting power is wasted.

If that is the case, this would be QA at best.

@OpenCoreCH, please review.

[OpenCoreCH]

Ah yes you are right, this finding does not mention the withdrawal that does not work, but the zero voting power. Yes I think that is a QA finding. It requires specific user actions (increasing lock duration) and even if it happens, it is easily recoverable (removing the delegation, as the lock of the user is longer per definition in this case).

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-b