veRWA - ADM's results

Lines of code

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

Vulnerability details

Impact

Due to a check requiring users only be able to delegate to others or themselves with longer lock times and verwa's restrictions of all changes increasing lock times by 5 years users may be forced to remain delegated to someone they do not wish to be or extend their lock much longer than they wish.

Proof of Concept

If a user does not delegate to another user who started their lock during the same epoch they will be unable to undelegate back to themselves without extending their own lock. This is not much of an issue if they wish to do so early in the lock period but can become a problem if they wish to delegate to themselves after a longer period of time. i.e.

1. Bob creates lock in week 1.
2. Dave create lock in week 2 & Bob delegates to Dave.
3. 3 years pass and Bob decides he wishes to undelegate his votes back to himself and calls delegate(msg.sender) but the call will fail due to the check in VotingEscrow#L384:

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

4. In the original FiatDAO contracts a user would be able to just extend their lock by one week to get around this however any changes in the verwa contract results in an extension of 5 years which the user may not want extend their lock by such a long time just to be able to undelegate.

The undelegate fail can be shown by modifying the test testSuccessDelegate to:

    function testSuccessDelegate() public {
        // successful delegate
        testSuccessCreateLock();
        vm.warp(8 days); // warp more than 1 week so both users are not locking in same epoch
        vm.prank(user2);
        ve.createLock{value: LOCK_AMT}(LOCK_AMT);
        vm.prank(user1);
        ve.delegate(user2);
        (, , , address delegatee) = ve.locked(user1);
        assertEq(delegatee, user2);
        (, , int128 delegated, ) = ve.locked(user2);
        assertEq(delegated, 2000000000000000000);
    }

and running: forge test --match testSuccessUnDelegate

Tools Used

Manual Review

Recommended Mitigation Steps

Modify VotingEscrow#L384 to:

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

which will allow users to delegate either to longer locks or undelegate back to themselves.

Assessed type

Timing