veRWA - nemveer's results

Lines of code

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

Vulnerability details

Details

Couple of points to note,

1. While delegating to someone, there'a a condition in delegate(), which allows delegation to only longer lock, i.e. Delegator can only delegate to a longer lock, i.e. to.end>from.end

VotingEscrow.delegate#L384

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

2. Withdraw is only possible when the delegatee == msg.sender

VotingEscrow.withdraw#L331

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

3. increaseAmount resets the lock end to 5 years

VotingEscrow.increaseAmount#L301

    newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);

Impact

• Delegatee can front-run the delegator's delegate() transaction with increaseAmount and prevent him from delegating to someone else. Thus, keeping the voting power to himself.
• Delegatee can front-run the delegator's withdraw() transaction with increaseAmount and prevent him from withdrawing Canto. Thus again, keeping the voting power to himself.

Proof of Concept

Let's see how Bob(malicious user) can prevent Alice from withdrawing. For simplicity let's assume the Locking period of 10 days

• Alice and Bob creates lock
  • Alice.locked.end = 10 days
  • Bob.locked.end = 10 days
• Alice delegates to Bob
  • Bob.locked.end = 10 days
  • Alice.locked.end = 10 days
• After 9 days,
  • Bob.locked.end = 1 days
  • Alice.locked.end = 1 days
  • Alice now wants to withdraw her Cantonext day. So she decides to undelegate(delegate to herself) which is necessary for withdraw.
  • She sends delegate(Alice.address) transaction.
  • Bob see this txn and decide to frontrun it with increaseAmount(1) passing 1 wei of canto as msg.value
    • This results in reseting of bob's lock end
    • Bob.locked.end = 10 days
  • Now Alice's delegate txn will revert due to to.end > from.end == false(1 day > 10 days == false)
• And thus she won't be able to withdraw her Canto

Tools Used

Manual Review

Recommended Mitigation Steps

I am not 100% sure what's the purpose of this condition in the delegate. According to me below condition is sufficient.

    require(to.end > block.timestamp);

Assessed type

DoS

Lines of code

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

Vulnerability details

Details

Let's first understand how the process of delegation, withdraw and increaseAmount works

1. Rule 1: To delegate to some user A, A.lock.end needs to be longer than msg.sender.lock.end and A.Lock.end > block.timestamp(Lock mustn't be expired)

VotingEscrow.delegate#L383-L384

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

2. Rule 2: To withdraw the locked Canto, delegatee must be msg.sender

VotingEscrow.withdraw#L331

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

3. Rule 3: Locking period is fixed, i.e. 5 years. You can't increase the lock.end directly but calling increaseAmount will reset it to block.timestamp + 5 years

VotingEscrow.increaseAmount#L301

    newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);

Now consider userA and userB.

userA has delegated to userB, which means userB.lock.end >= userA.lock.end And userA wants to withdraw his Canto in 2 different scenarios.

1. userA.lock.end > block.timestamp, so userA's lock is not expired yet

• To withdraw userA.lock.delegatee == userA.address, so, userA need to undelegate from userB to himself.
  • But userB.lock.end >= userA.lock.end as per Rule2, so userA can't delegate to himself because of Rule 1
  • So, he has no option left but to call increaseAmount with minimum amount possible and increase his lock time to another 5 years. So won't be able to withdraw until another 5 years

2. userA.lock.end < block.timestamp, so userA wants to withdraw after his lock is expired.

• To withdraw userA.lock.delegatee == userA.address as per Rule2, so, userA need to undelegate from userB to himself.
  • But to delegate to self, A.Lock.end > block.timestamp as per Rule1. So againhe won't be able to call delegate
  • Again no option left but to call increaseAmount, reset the lock.end and then call delegate(userA.address).
  • Again won't be able to withdraw until next 5 years

Impact

After delegating to someone else, User must need to call increaseAmount and wait for another 5 years to withdraw

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

I am not sure about the reasoning behind a fixed lock time of 5 years. It doesn't provide flexibility and convenience to users. But I am sure there must be reasons for choosing it. For now I think removing the below condition works.

VotingEscrow.delegate#L384

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In _checkpoint, slope is calculated and then multiplied with time delta to calculate the bias

VotingEscrow._checkpoint#L130-L131

                userOldPoint.slope = _oldLocked.delegated / int128(int256(LOCKTIME));
                userOldPoint.bias = userOldPoint.slope * int128(int256(_oldLocked.end - block.timestamp));

Here, division before multiplication happens.

If _oldLocked.delegated is less than LOCKTIME then slope = 0 and then bias = 0. Which lead to loss of user's voting power even if user has locked some Canto

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Multiply the _oldLocked.delegated with some multipier to calculate slope and then divide the bias with that multiplier

Assessed type

Other