veRWA - kaden's results

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/498a3004d577c8c5d0c71bff99ea3a7907b5ec23/src/LendingLedger.sol#L135

Vulnerability details

Impact

Users can expose themselves to only a small fraction of the risk involved with being deposited while still receiving 100% of the rewards, or even doubling their rewards for a given amount of liquidity.

Proof of Concept

When a deposit is made to a lending market, LendingLedger.sync_ledger is called by the market, syncing their balance at the current epoch.

// sync_ledger()
uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
int256 updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta;
require(updatedLenderBalance >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
lendingMarketBalances[lendingMarket][_lender][currEpoch] = uint256(updatedLenderBalance);

When users later claim, it pays them out if they had a position during that epoch according to lendingMarketBalances.

// claim()
for (uint256 i = claimStart; i <= claimEnd; i += WEEK) {
	uint256 userBalance = lendingMarketBalances[_market][lender][i];
	uint256 marketBalance = lendingMarketTotalBalance[_market][i];
	RewardInformation memory ri = rewardInformation[i];
	require(ri.set, "Reward not set yet"); // Can only claim for epochs where rewards are set, even if it is set to 0
	uint256 marketWeight = gaugeController.gauge_relative_weight_write(_market, i); // Normalized to 1e18
	cantoToSend += (marketWeight * userBalance * ri.amount) / (1e18 * marketBalance); // (marketWeight / 1e18) * (userBalance / marketBalance) * ri.amount;
}

This means that users receive the full epoch reward even if they had only deposited at the very end of the epoch.

This allows malicious users to deposit at the end of epochs and withdraw at the start, later receiving the same amount of rewards as if they held their deposit the entire time. They can repeat this for every epoch, only being exposed to the associated risk for a couple blocks per epoch, while receiving the same amount of rewards as someone being exposed to risk for the full duration.

Furthermore, they could alternate their liquidity between markets such that they get the full reward of having their full amount of liquidity deposited on both markets, effectively doubling their intended reward for the amount of liquidity at risk.

As a result of attackers doubling their rewards and the simplicity of this attack, it's likely to become commonplace, leading to unknowing users being at a significant disadvantage.

Recommended Mitigation Steps

It's recommended that deposits only set the user's balance for the next epoch while withdrawals still set the user's balance for the current epoch. For example,

// sync_ledger()
uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
uint256 nextEpoch = ((block.timestamp + WEEK) / WEEK) * WEEK;

int256 updatedLenderBalance
if (_delta > 0) {
	// deposit, apply to next epoch
	updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][nextEpoch]) + _delta;
} else {
	// withdrawal, apply to current epoch
	updatedLenderBalance = int256(lendingMarketBalances[lendingMarket][_lender][currEpoch]) + _delta;
}
require(updatedLenderBalance >= 0, "Lender balance underflow"); // Sanity check performed here, but the market should ensure that this never happens
lendingMarketBalances[lendingMarket][_lender][currEpoch] = uint256(updatedLenderBalance);

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/498a3004d577c8c5d0c71bff99ea3a7907b5ec23/src/GaugeController.sol#L211

Vulnerability details

Impact

Users can reuse their voting power by voting, then delegating to an account they control, and voting again, repeating this process to have virtually infinite voting power.

Proof of Concept

When a user creates a lock, it _checkpoints the slope and lock end, which is later used in GaugeController.vote_for_gauge_weights to determine the user's bias and increase the gauge weight and sum accordingly.

// vote_for_gauge_weights()
(
	,
	/*int128 bias*/
	int128 slope_, /*uint256 ts*/

) = ve.getLastUserPoint(msg.sender);

...

uint256 lock_end = ve.lockEnd(msg.sender);

...

VotedSlope memory new_slope = VotedSlope({
	slope: (slope * _user_weight) / 10_000,
	end: lock_end,
	power: _user_weight
});

We can see in VotingEscrow._checkpoint, that the user Point is calculated using the delegated amount.

// _checkpoint()
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));
}

...

userPointHistory[_addr][uEpoch + 1] = userNewPoint;

The problem with all this is that there is nothing to invalidate a users prior votes once they delegate. As a result, they can delegate to an account they control, _checkpointing that accounts' Point, which is then used when they vote with vote_for_gauge_weights from the delegated account, thereby effectively doubling their voting power.

Furthermore, the user can then repeat this process by delegating a new account they control and voting yet again, ad infinitum.

Recommended Mitigation Steps

It's not clear how we can solve this problem without removing delegation logic unless we were to disable delegation for users who have an existing voting weight or who have previously delegated. It is otherwise very difficult to keep track of the share of voting weight of every user throughout every delegated vote.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If a user's lock expires while delegated to another user, they cannot undelegate their tokens and therefore cannot withdraw, permanently locking their tokens in the contract.

Proof of Concept

Consider a circumstance in which a user has delegated to another user. Their lock has expired and they intend to withdraw their locked tokens.

In VotingEscrow.withdraw, you cannot withdraw your lock if you're currently delegating to another account.

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

But in delegate, you can't set a new delegate as an expired account.

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

This logic makes sense in the case that you are actually delegating to another account, but the logic is the same here regardless of whether you're delegating or undelegating.

As a result, in the above example, the user cannot withdraw their position even though it has reached the end time.

Normally, users can extend their positions using increaseAmount, which resets the lock period, but this will revert because you can't execute it on expired locks.

// increaseAmount()
require(locked_.end > block.timestamp, "Lock expired");

Since we cannot undelegate with an expired lock and we cannot extend the end time of the lock in any way, the users funds are permanently locked.

Recommended Mitigation Steps

It's recommended that we allow undelegation of expired locks. e.g.

// delegate()
require(toLocked == locked_ || toLocked.end > block.timestamp, "Delegatee lock expired");

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Any time a user delegates to another user, they will have to at some point reset their lock time to undelegate. As a result, delegatees can, either intentionally or unintentionally, increase their lock time to prevent delegators from undelegating, preventing withdrawal.

Proof of Concept

In VotingEscrow.delegate, users may only delegate to locks with a later end time.

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

The problem with this is that the same logic applies when undelegating, which must be done to withdraw.

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

Since the lock we're delegating to must be longer than the lock we're delegating from, and we also have to undelegate from that lock with matching logic as if we're delegating, we must at some point reset our lock time. Because the lock can only reset to an end date of 5 years from the current time, and the user may delegate at any point throughout the lifecycle of their lock, this logic could easily force the user to lock their tokens for almost double the initially expected lock time.

Consider for example, a circumstance in which a user delegates their lock until shortly before it expires before going to undelegate their lock and suddenly realizes that they must extend their lock period by 5 years.

Furthermore, it's possible that the delegatee extends their lock period, either for their own sake or to prevent the delegator from undelegating, causing the user to have their lock extended indefinitely.

Recommended Mitigation Steps

Mitigating this issue is difficult because allowing a user to undelegate with a closer end time means that users can execute an attack by which they delegate to another, newer account which they control so that they can prevent voting decay. At this point I'm not clear on any solutions other than removing delegation functionality or somehow including delegator voting decay into the delegated amount such that they can't perform the attack to artificially prevent voting decay.

Assessed type

DoS

_get_sum call at end of GaugeController.vote_for_gauge_weight is a noop

_get_sum only runs a loop iteration if time_sum <= block.timestamp.

// _get_sum()
uint256 t = time_sum;
Point memory pt = points_sum[t];
for (uint256 i; i < 500; ++i) {
	if (t > block.timestamp) break;

And it the return value is unused.

// vote_for_gauge_weight
_get_sum(); // @audit: unused return value

Since we executed _get_sum earlier during the function execution, we set time_sum as > block.timestamp.

// _get_sum
if (t > block.timestamp) time_sum = t;

As a result, calling _get_sum here is a noop.

Intra-epoch lock timing can result in +/- 1 epoch of rewards

LOCKTIME is 1825 days, which amounts to 260.7142857142857 epochs.

// VotingEscrow.sol
uint256 public constant LOCKTIME = 1825 days;

Creating a lock sets your unlock time as the _floorToWeek value of 1825 days from the current timestamp.

// createLock
uint256 unlock_time = _floorToWeek(block.timestamp + LOCKTIME); // Locktime is rounded down to weeks

As a result, creating a lock in the last ~30% of an epoch results in an extra epoch considering to the first ~70%.

Increasing the locktime to 1827 days would be cleanly divisible by epochs, resulting in the same amount of epochs regardless of lock timing. This is also still well aligned with 5 years when you consider leap years, since any 5 year period has at least one leap year, if not two, 5 years would be at least 1826 days, if not 1827 depending on timing.