veRWA - 3docSec's results

Lines of code

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

Vulnerability details

The validation checks that VotingEscrow has in place around delegation prevent some invalid cases from happening. However, the combination of these checks happens to prevent the withdrawal of funds that reached maturity while delegated.

Impact

Users who delegated their funds to a delegatee, and let their own lock come to expiration, will see their funds permanently locked in the VotingEscrow contract.

Proof of Concept

The following Foundry test shows the point:

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

        // Alice locks some CANTO
        vm.prank(alice);
        ve.createLock{value: 10 ether}(10 ether);

        // Bob locks some CANTO
        vm.prank(bob);
        ve.createLock{value: 10 wei}(10 wei);

        // Alice delegates Bob
        vm.startPrank(alice);
        ve.delegate(bob);

        // Both Alice's and Bob's veCANTO reach maturity
        vm.warp(currentTs + LOCKTIME);

        // Alice can't withdraw because the delegation is in place ...
        vm.expectRevert("Lock delegated");
        ve.withdraw();

        // ... and she can't undo the delegation because her own lock is expired ...
        vm.expectRevert("Delegatee lock expired");
        ve.delegate(alice);

        // ... and she can't renew the lock
        vm.expectRevert("Lock expired");
        ve.increaseAmount{value: 1 wei}(1 wei);

        // Alice is in a cul-de-sac 😰 
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Relax the validation steps, possibly removing the "Delegatee lock expired" when delegating to self:

            // Then, update delegatee's lock and voting power (checkpoint)
            locked_ = locked[delegatee];
            require(locked_.amount > 0, "Delegatee has no lock");
-            require(locked_.end > block.timestamp, "Delegatee lock expired");
+            require(locked_.end > block.timestamp || _addr == msg.sender, "Delegatee lock expired");
            newLocked = _copyLock(locked_);
            newLocked.delegated += int128(int256(_value));
            locked[delegatee] = newLocked;

Assessed type

Other

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