veRWA - carrotsmuggler's results

Lines of code

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

Vulnerability details

Impact

Users are allowed to delegate their voting power to other delegates, who will manage them to redirect emissions to certain pools. In order to withdraw the underlying tokens, users can withdraw from the escrow contract once their lock period is over. However, to do this, the need to remove the delegation first.

For users to remove delegated power, their own lock must be longer or equal to the lock of the previous delegated. Users can increase their own lock duration by calling increaseAmount and passing even 1 wei will reset the lock to 5 years. now, users can redelegate tokens to themselves, and then after 5 years, they can withdraw.

The issue arises when the user's lock expires before redelegation. Imagine a user creates a 5 year lock. They then delegate their voting power to some other address. After 5 years, the user's lock has expired. If the user wants to increase their lock time, they call increaseAmount, but this function reverts due to the require statement shown below.

require(locked_.end > block.timestamp, "Lock expired");

So if a user's position is already expired, they cannot increase the duration. If they cannot increase the duration, they cannot redelegate the voting power back to themselves. This is because the toLocked address's lock must not be expired, which in this case is the user themselves.

require(toLocked.end > block.timestamp, "Delegatee lock expired");

Tuhs users cannot re-delegate to themselves. Users are also not able to withdraw, since withdrawing requires voting power to be delegated to themselves.

require(locked_.delegatee == msg.sender, "Lock delegated");

So the user cannot call any function since all of them revert. Thus the user is essentially locked out of the system, losing their tokens forever.

The main issue is that when the contract was forked from the FiatDao contract, the developers merged the increaseUnlockTime and increaseAmount into the same function increaseAmount. The older code in increaseLockTime allowed users to increase their lock times even on expired locks. The increaseAmount function does not let users interact with the function at all. The removal of this function also removed the ability of the users to increase lock duration by any means of already expired positions.

Proof of Concept

A POC has been developed to show the above stated issue. The steps carried out are:

1. User creates lock of 5 years.
2. User delegates to a fresh lock after 1 year.
3. After 4 years, the user's position has expired, but the delgatee still has a live position.
4. Every function the user can call reverts. This is shown using vm.expectRevert

function testAttack() public {
        // user1 -> victim
        // user2 -> delegatee
        testSuccessCreateLock();
        vm.warp(block.timestamp + 365 days);
        vm.prank(user2);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);
        vm.prank(user1);
        ve.delegate(user2);
        vm.warp(block.timestamp + 4 * 365 days);
        // Assert user1/victim end timestamp has passed
        assert(block.timestamp > ve.lockEnd(user1));

        // Assert all functions revert
        vm.startPrank(user1);
        vm.expectRevert("Delegatee lock expired");
        ve.delegate(user1);
        vm.expectRevert("Lock delegated");
        ve.withdraw();
        vm.expectRevert("Lock expired");
        ve.increaseAmount{value: 1 ether}(1 ether);
        vm.stopPrank();
    }

This shows that the user is locked out of all operations, and thus loses their tokens.

Tools Used

Foundry

Recommended Mitigation Steps

Add the functionality to increase lock time from the FiatDao contract in here. One way can be to implement the function. Another could be to relax the restrictions on the existing increaseAmount function for expired locks since the lock will be updated anyways.

Assessed type

DoS