veRWA - 0xDING99YA's results

Lines of code

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

Vulnerability details

Impact

When a user create a lock and then delegate the votes to another user, and after the lockEnd time when the user want to withdraw the transaction will revert, there is no way to get the token back and the fund is locked forever.

Proof of Concept

In withdraw() in VotingEscrow.sol, when withdrawing it requires the delegate is the msg.sender itself:

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

And if a user want to delegate to any user, that user (including the user self) lockEnd must larger than current timestamp:

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

So it's possible that user1 delegate to user2, when user1 wants to withdraw after lockEnd, user1 must delegate back to self, however, this is impossible since delegate always check user1.lockEnd > current timestamp.

POC in Foundry is below, just added it to VotingEscrow.t.sol and run:

function testWithdrawRevert() public {
    // user1 create lock
    vm.prank(user1);
    ve.createLock{value: LOCK_AMT}(LOCK_AMT);
    // user1 lockEnd is 157248000

    // make the block.timestamp to 10000000, then user2 create lock
    vm.warp(10000000);
    vm.prank(user2);
    ve.createLock{value: LOCK_AMT}(LOCK_AMT);
    // user2 lockEnd is 167529600

    // user1 delegate to user2
    vm.prank(user1);
    ve.delegate(user2);

    // make the block.timestamp to 157248001, which is 1s larger than user1.lockEnd
    vm.warp(157248001);

    // user1 cannot withdraw directly since delegatee is not the user self
    vm.expectRevert("Lock delegated");
    vm.prank(user1);
    ve.withdraw();

    // user1 can't delegate back to self, but withdraw require(delegatee == msg.sender), so user1 cannot get token back forever
    vm.expectRevert("Delegatee lock expired");
    vm.prank(user1);
    ve.delegate(user1);
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Enable users to delegate back to self even after the lockEnd

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

After a user delegate the balance to another user, the user can't delegate back to self unless increase own balance.

Proof of Concept

In delegate() in VotingEscrow.sol, it requires the lockEnd of new delegatee > lockEnd of current delegatee:

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

So if a user delegate balance to another address, to delegate back the user has to call increaseAmount() to update the lockEnd

Tools Used

Manual Review

Recommended Mitigation Steps

This is a weird design, not sure the reason to do this. Remove the requirement.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

After a user making a deposit, the user will incur an immediate loss in the balance (voting power).

Proof of Concept

By design, the balance of a user will decrease linearly by time. For example, if a user deposit 1 ether, then the 1 ether balance will decrease by each single second. However, at the same timestamp of depositing the balance should still be 1 ether, this is not true in current implementation.

if (_newLocked.end > block.timestamp && _newLocked.delegated > 0) {
            userNewPoint.slope = _newLocked.delegated / int128(int256(LOCKTIME));
            userNewPoint.bias = userNewPoint.slope * int128(int256(_newLocked.end - block.timestamp));
}

As we can see in _checkPoint(), slope is delegated/LOCKTIME, then current bias is slope*(lockEnd-block.timestamp), the question is when making a deposit the lockEnd is not always block.timestamp+LOCKTIME, it will round to the nearest multiple of WEEK, which can result in a loss in bias and the bias will be less than the current deposit.

Foundry POC is below, just added it to VotingEscrow.t.sol.

function testDepositImmediateLoss() public {
    // We choose a random block timestamp which is not a multiple of WEEK
    vm.warp(604799);

    // user1 make a deposit
    vm.prank(user1);
    ve.createLock{value: LOCK_AMT}(LOCK_AMT);

    // In the same block timestamp user's balance is already less than the value deposited, which is 1 ether
    uint256 user1Balance = ve.balanceOf(user1);
    assertLt(user1Balance, 1 ether);
}

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Make sure the user won't loss balance immediately at the same timestamp of deposit.

Assessed type

Other