veRWA - Jorgect's results

Lines of code

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

Vulnerability details

Impact

Users can not undelegate his position, this is because the contract is requiring that the end of the new delegatee has to be greater than the old delegatee.

to understand this let´s analise the function delegate:

file:/src/VotingEscrow.sol
 function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        if (delegatee == msg.sender) {
            // Delegate
            fromLocked = locked_;
            toLocked = locked[_addr];
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;
        } 
        ...
        require(toLocked.amount > 0, "Delegatee has no lock");
        require(toLocked.end > block.timestamp, "Delegatee lock expired");
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        ...
    }

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

This function has the next require:

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

When a user delegate his power his delegatee end time have to be greater than the user; in this case user = fromLocked and delegatee = toLocked, and this is fine but what about is a user want undelegate? he can not because now user = toLocked and delegatee = fromLocked and user have less end than the delegatee.

however there is a way to undelegate, the user has to call increaseAmount function passing 1 wei and increasing his end time, but user is no aware of this and is not how the contract should work.

Proof of Concept

The two function below were taked from the test of the contract the only modification that i did was add time betwen two createLock functions.

 function testSuccessDelegate() public {
        // successful delegate
        testSuccessCreateLock();
        vm.warp(block.timestamp + 100 days); // modification
        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);
        console.log(ve.numbercall());
    }

function testSuccessUnDelegate() public {
        // successful undelegate
        testSuccessDelegate();

        vm.prank(user1);
        ve.delegate(user1);
        (,,, address delegatee) = ve.locked(user1);
        assertEq(delegatee, user1);
    }

testSuccessUnDelegate is gonna fail but we analize the transaction is reverting in :

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

This test in the repository is passing because both are created in the same transaction.

Tools Used

manual, foundry

Recommended Mitigation Steps

FIDO DAO has the same logic in the delegate function:

 if (_addr == msg.sender) {
            _undelegate();
            return;
        }

Consider apply his approach. other solution is think in remove the require stament

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

User can not undelegate or redelegate is a malicius delegatee front run the user transaction increasing his amount of time to end calling increaseAmount.

Proof of Concept

We can explore this analyzing both functions:

file:src/VotingEscrow.sol
    function delegate(address _addr) external nonReentrant {
        
        ...
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;
        } else {
            // Re-delegate
            fromLocked = locked[delegatee];
            toLocked = locked[_addr];
            // Update owner lock if not involved in delegation
            locked[msg.sender] = locked_;
        }
        ...
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        ...
    }

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

This function require that old delegatee end be less than the new delegatee.

Now analyse the increaseAmount function:

file:src/VotingEscrow.sol
function increaseAmount(uint256 _value) external payable nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        LockedBalance memory newLocked = _copyLock(locked_);
        newLocked.amount += int128(int256(_value));
        newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);
        if (delegatee == msg.sender) {
            // Undelegated lock
            action = LockAction.INCREASE_AMOUNT_AND_DELEGATION;
            newLocked.delegated += int128(int256(_value));
            locked[msg.sender] = newLocked;
            _checkpoint(msg.sender, locked_, newLocked);
        } else {
            // Delegated lock, update sender's lock first
            locked[msg.sender] = newLocked;
           ...
        }
        emit Deposit(msg.sender, _value, unlockTime, action, block.timestamp);
    }

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

This function is increasing the time of the end by 5 years, so if a delegatee see in the mempool that he is gonna be undelegate or re delegate, he can front run that transaction calling increaseAmount passing 1 wei and increasing his end for 5 years more breaking the require(toLocked.end >= fromLocked.end, "Only delegate to longer lock") in delegate function.

Tools Used

manual

Recommended Mitigation Steps

One recomendation is follow what FIAT DAO is doing in his VotingEscrow contract, other approach that the project can take is eliminate the increasing of the time end when user call increasAmount, there is not incentive to the user to call that function if he is gonna lock for 5 years more

Assessed type

Other