veRWA - lsaudit's results

Lines of code

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

Vulnerability details

Impact

User A delegates his lock to user B. User B calls increaseAmount on his lock, thus locked.end is being updated. Now, user A wants to undelegate his lock (from user B), however, since locked.end had been updated, function reverts on require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");. User A lock stuck in delegated state and cannot be undelegated.

Proof of Concept

The attack scenario can only occur during the week change (the locked.end has to be updated). User A needs to delegate his lock on the X week, while user B needs to call increaseAmount on X+1 week (otherwise, locked.end won't be updated to the new value). To demonstrate the issue more clearly (without waiting the whole week), let's modify line 31 of VotingEscrow from 7 days to 7 seconds:

 uint256 public constant WEEK = 7 seconds;

That way, we will be able to demonstrate the issue within 7-seconds PoC.

Since VotingEscrow.sol does not import any additional files, let's directly copy-paste it into Remix IDE. Afterwards, let's modify line 31: from uint256 public constant WEEK = 7 days; to uint256 public constant WEEK = 7 seconds; and compile and deploy.

We will be using two accounts:

User A: 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
User B: 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2

1. Create locks:

Set account 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4 and create lock with any value, e.g.: createLock(100);
Set another account 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2 and create another lock with any value, e.g.: createLock(100);

2. Delegate A's lock to B:

Set account 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4 and run delegate(0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2)

3. You can confirm, that locked.end is the same for both accounts by running locked on both addresses:

locked[0x5B38Da6a701c568545dCfcB03FcB875f56beddC4]: 
    0:
    int128: amount 100
    1:
    uint256: end 1849282403
    2:
    int128: delegated 0
    3:
    address: delegatee 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2

locked[0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2]:
    0:
    int128: amount 100
    1:
    uint256: end 1849282403
    2:
    int128: delegated 200
    3:
    address: delegatee 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2

4. Wait at least 7 seconds and increase amount from user B:

Set account 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2 and call increaseAmount with any value, e.g.: increaseAmount(123);

5. Now, confirm, that B's locked.end has been indeed updated.

locked[0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2]
    0:
    int128: amount 223
    1:
    uint256: end 1849282516
    2:
    int128: delegated 323
    3:
    address: delegatee 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2

6. Try to undelegate A:

Set account 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4 and call delegate(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4).

Function reverts:

The transaction has been reverted to the initial state.
Reason provided by the contract: "Only delegate to longer lock".
Debug the transaction to get more information.

This issue occurs because of line 384:

File: VotingEscrow.sol

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

When user B called increaseAmount after 7 seconds (in real PoC, that needs to be 7 days), locked.end was updated. However, locked.end on the A's lock still contains old value, thus it's impossible to undelegate it.

The only possible way for user A to undelegate is, unfortunately, increasing his locked.end value. He can do it, only by calling increaseAmount function. After calling increaseAmount, he will be able to undelegate, thus locked.end will be updated. This scenario, however, is not desirable:

• user needs to spend additional funds to call increaseAmount - even if it's just calling increaseAmount(1) - it's still unnecessary cost for A - to undelegate his lock

• calling increaseAmount increases the locktime:

File: VotingEscrow.sol

function increaseAmount(uint256 _value) external payable nonReentrant {
(...)
newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);

This will basically extend the lock to another 1825 days (LOCKTIME). This is the main issue why I've evaluated this report as High. Let's consider a user who created a lock 4 years ago. There is 1 year left till user will be able to withdraw from the lock (lock time is fixed to 5 years). Now delegatee performs the attack. User is not able to undelegate his lock. It's crucial to notice, that user cannot wait the remaining 1 year and withdraw, because it's not possible to withdraw from the delegated lock:

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

The lock needs to be undelegated first. To do so, user needs to call increaseAmount. increaseAmount updates the lock time, which is now 5 years after the increase call and 9 years (4 years which had passed + 5 years from the increaseAmount) after the initial lockup. Because of the attack - user lost at least 1 wei (to call increaseAmount(1)) and his funds are locked for additional 5 years.

Tools Used

Manual code review and Remix IDE

Recommended Mitigation Steps

Undelegating lock should not have additional restriction: toLocked.end >= fromLocked.end. Move require(toLocked.end >= fromLocked.end, "Only delegate to longer lock"); into if-condition, so this check will be verified only when user delegates his lock to others, but do not check this condition if user is trying to undelegate:

Change line 384 in VotingEscrow.sol from:

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

to:

if (msg.sender != _addr)  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#L356-L387

Vulnerability details

Impact

User who accidentally delegates his lock to the newer one, will get his lock stuck. User won't be able to undelegate his lock, because function delegate will always revert.

Please notice, that this is the different issue than previously reported: "Delegatee is able to forbid user from undelegating". In the previous scenario, it's delegatee who performs the attack by calling increaseAmount on their lock. In the current scenario - delegatee interaction is not required. It's user who has accidentally or without realizing the consequences (this scenario is not documented anywhere) delegated his lock to a newer lock - thus makes his lock impossible to undelegate.

Proof of Concept

Two locks were created, each with 1 week difference (1849478400 - 1848873600 = 604800 seconds = 7 days):

locked[0x5B38Da6a701c568545dCfcB03FcB875f56beddC4]:
    0:
    int128: amount 123
    1:
    uint256: end 1848873600
    2:
    int128: delegated 123
    3:
    address: delegatee 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4

locked[0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2]:
    0:
    int128: amount 123
    1:
    uint256: end 1849478400
    2:
    int128: delegated 123
    3:
    address: delegatee 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2

The lock 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4 was created first, then the lock 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2 was created after one week.

Now, we will delegate lock 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4 to 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2:

From 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, call: delegate(0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2)

Now, when we will try to undelegate this lock, function will revert:

From: 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4 call: delegate(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4)

The transaction has been reverted to the initial state.
Reason provided by the contract: "Only delegate to longer lock".
Debug the transaction to get more information.

This issue occurs, because of line 384:

File: VotingEscrow.sol

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

The only way to undelegate the lock - is to call increaseAmount on the lock, which will modify its locked.end date.

However, this scenario is not desirable:

• calling increaseAmount requires spending additional funds, even if it's just calling increaseAmount(1) - it's still unnecessary cost for undelegating the lock

• calling increaseAmount increases the locktime:

File: VotingEscrow.sol

function increaseAmount(uint256 _value) external payable nonReentrant {
(...)
newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);

Calling increaseAmount extends the lock to another 5 years (LOCKTIME = 1825 days) Because of this, I've evaluated this issue as High.

E.g.: User created a lock 4 years ago. There is 1 year left till he will be able to withdraw from the lock (based on the code-base and documentation: lock time is fixed to 5 years). Now, user decides to delegate his lock and he delegates it to the lock which had been created at least 1 week after the user's lock (user's older lock is being delegated to the newer lock). Because of this - user is not able to undelegate the lock. Even after one year passes, user won't be able to withdraw from the lock, because to withdraw - lock cannot be delegated:

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

To undelegate the lock - user needs to call increaseAmount, which updates the lock time. Now, lock is being locked for additional 5 years (5 years after the increaseAmount call and 9 years after the initial lockup).

Tools Used

Manual code review and Remix IDE

Recommended Mitigation Steps

When user tries to undelegate his lock, there should not be additional require: toLocked.end >= fromLocked.end. This require statement should be used only when user delegates their lock to others, but not when user tries to undelegate own lock.

Modify line 384 from:

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

to:

if (msg.sender != _addr)  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/LendingLedger.sol#L131 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/LendingLedger.sol#L204-L210

Vulnerability details

Impact

governance can remove any Lending Market from whitelist - thus calling sync_ledger and withdraw cNOTE won't be possible. Malicious governance may use this to block others from withdrawing cNOTEs. Even if the governance is trustworthy, having a possible attack vector like this in the contract may have a negative impact of the protocol's reputation and undermine the users' trust in the contract.

Proof of Concept

governance can call whiteListLendingMarket and remove a market from a whitelist. This removal will make withdrawal of cNOTE impossible:

File: LendingLedger.sol

require(lendingMarketWhitelist[lendingMarket], "Market not whitelisted");

sync_ledger checks if market is on the whitelist, no matter if it's called with positive (_delta parameter) or negative (_delta parameter) value. While negative _delta suggests, there is a cNOTE withdrawal, it still won't be possible.

Malicious governance may remove market from the whitelist and disallow users to withdraw cNOTEs.

Tools Used

Manual code review

Recommended Mitigation Steps

Being whitelisted should not affect withdrawals. require(lendingMarketWhitelist[lendingMarket], "Market not whitelisted") should only be called when _delta is positive (depositing cNOTE), but not during withdrawals (_delta negative):

if (_delta > 0) require(lendingMarketWhitelist[lendingMarket], "Market not whitelisted");

That way, governance won't be able to block everyone from withdrawals by removing market from the whitelist.

Assessed type

Other