Delegated votes are locked when owner lock is expired

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L331
https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L371-L374
https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L383

Vulnerability details

Impact

In delegate() of VoteEscrow.sol, a user is able to delegate their locked votes to someone else, and undelegate (i.e. delegate back to themselves). When the user tries to re-delegate, either to someone else or themselves, the lock must not be expired. This is problematic because if a user forgets and lets their lock become expired, they cannot undelegate. This blocks withdrawal, which means their tokens are essentially locked forever.

Proof of Concept

To exit the system, Alice must call withdraw(). However, since they've delegated, they will not be able to.

function withdraw() external nonReentrant {
	...
	require(locked_.delegatee == msg.sender, "Lock delegated");
	...
}

To re-delegate to themselves (undelegate), they call delegate(alice.address). However, there is a check to see if toLocked.end has expired, which would be true since it would point to Alice's lock.

function delegate(address _addr) external nonReentrant {
	LockedBalance memory locked_ = locked[msg.sender];
	...
	LockedBalance memory fromLocked;
	LockedBalance memory toLocked;
	locked_.delegatee = _addr;
	if (delegatee == msg.sender) {
		...
	// @audit this else if will execute
	} else if (_addr == msg.sender) {
		// Undelegate
		fromLocked = locked[delegatee]; // @audit Delegatee
		toLocked = locked_; // @audit Alice's lock
	}
	...
	require(toLocked.end > block.timestamp, "Delegatee lock expired");

This is a test to be added into VoteEscrow.t.sol. It can be manually run by executing forge test --match-test testUnSuccessUnDelegate.

function testUnSuccessUnDelegate() public {
	testSuccessDelegate();
	vm.warp(ve.LOCKTIME() + 1 days);

	// Try to undelegate
	vm.startPrank(user1);
	vm.expectRevert("Delegatee lock expired");
	ve.delegate(user1);

	// Still user2
	(, , , address delegatee) = ve.locked(user1);
	assertEq(delegatee, user2);
}

Tools Used

Manual

Recommended Mitigation Steps

Consider refactoring the code to skip toLocked.end > block.timestamp if undelegating. For example, adding a small delay (e.g., 1 second) to the lock end time when a user undelegates.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as duplicate of #223

[c4-pre-sort]

141345 marked the issue as duplicate of #112

[c4-judge]

alcueca marked the issue as duplicate of #82

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as satisfactory

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as duplicate of #211

[c4-judge]

alcueca marked the issue as selected for report

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[alcueca]

This vulnerability, if not found, would have meant that some users would have permanently lost assets in the for of voting power. While at that point the application owners would certainly warn users to not let their locks expire without undelegating, many users would not get the warning, as it is not that easy to make sure that every user is aware of something. The result is that time and again, users would get their tokens locked forever.

[OpenCoreCH]

Fixed in mkt-market/canto-neofinance-coordinator#5