veRWA - Topmark's results

Lines of code

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

Vulnerability details

Impact

Locked Account Owner will have funds Locked Forever Even after Expiration day has elapsed if Delegetee Loses Private key or refuse to withdraw

Proof of Concept

Analyzing this complex code base specifically the VotingEscrow.sol contract shows a complex delegation system but there is contradiction which would affect the ability of a Locked account owner to withdraw it funds even after the LOCK_TIME period has expired.

326.      function withdraw() external nonReentrant {
327.        LockedBalance memory locked_ = locked[msg.sender];
328.        // Validate inputs
329.        require(locked_.amount > 0, "No lock");
330.        require(locked_.end <= block.timestamp, "Lock not expired");
331.        require(locked_.delegatee == msg.sender, "Lock delegated");

A quick look at L331 of the withdraw(...) function above shows that Account owner ( msg.sender) must undelegated the delegatee (locked_.delegatee) back to his own address for the condition on L331 to pass and the only way to do that is the delegate(...) function at L356-L387.

356. function delegate(address _addr) external nonReentrant {
   ....
367.        if (delegatee == msg.sender) {
368.            // Delegate
369.            fromLocked = locked_;
370.            toLocked = locked[_addr];
371.        } else if (_addr == msg.sender) {
372.            // Undelegate
373.            fromLocked = locked[delegatee];
374.            toLocked = locked_;
375.        } else {
        .....
        }
382.        require(toLocked.amount > 0, "Delegatee has no lock");
383.        require(toLocked.end > block.timestamp, "Delegatee lock expired");
384.        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock"); <--- vulnerability point
   ....
387.    }

looking at the conditions inside the delegate function, since the user wants to undelegate back to himself, we are interested with the condition at L371 since _addr parameter would equal msg.sender, the content of this condition has a flaw as it contradicts the required condition on L384 which simply mandates the lock end period of the locked account to be updated to be longer that fromLocked.end value. but the problem is this same condition was passed when

1. Alice delegated to Bob ( Bob account end is longer)
2. Now Alice wants to undelegate Bob to Alice with same condition ( Alice must be longer but Bob account end is still longer and L384 condition will not pass this time around) you can't have it both ways! However there is a safe spot if Bob first withdraw his own funds which resets his account end back to zero based on L336

336.   newLocked.end = 0;

with this Alice can undelegate Bob to Alice as (Alice is now higher since Bob is now zero) But the big question is, what happens if Bob refuse to withdraw or loses his private key completely, then Alice funds is inaccessible or doomed as the case may be.

Tools Used

Solidity, Hardhat

Recommended Mitigation Steps

This vulnerability is centered on the condition on L384 of the VotingEscrow.sol contract which does not factor in when user wants to undelegate back to themselves, a good solution would be to have this condition specific to each of the condition at L387, L371 & L375, more importantly L371 which will have it own require conditions in opposite direction i.e

367.         if (delegatee == msg.sender) {
368.            // Delegate
369.            fromLocked = locked_;
370.            toLocked = locked[_addr];
        +++ require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
371.        } else if (_addr == msg.sender) {
372.            // Undelegate
373.            fromLocked = locked[delegatee];
374.            toLocked = locked_;
     +++    if(Bob has withdrawn ){
     +++ require(toLocked.end <= fromLocked.end, "Only delegate to Longer lock");//<--when Bob has withdrawn}
     +++   if( When Bob has not withdrawn ){
     +++ require(toLocked.end <= fromLocked.end, "Only delegate to Shorter lock");//<-Bob has not withdrawn}
375.        } else {
376.            // Re-delegate
377.            fromLocked = locked[delegatee];
378.            toLocked = locked[_addr];
379.            // Update owner lock if not involved in delegation
380.            locked[msg.sender] = locked_;
        +++ require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
381.        }
382.        require(toLocked.amount > 0, "Delegatee has no lock");
383.        require(toLocked.end > block.timestamp, "Delegatee lock expired");
384.     --- require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L54-L63 https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L82-L86 https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L116-L123

Vulnerability details

Impact

Code Application or Comment is Wrong in L54-L63 of the _checkpoint_lender(...) function in the LendingLedger.sol contract which could cause misleading functionality Error

Proof of Concept

https://github.com/code-423n4/2023-08-verwa/blob/main/src/LendingLedger.sol#L54-L63

54.  /// @param _forwardTimestampLimit Until which epoch (provided as timestamp) should the update be applied. If it is higher than the current epoch timestamp, this will be used.
55.    function _checkpoint_lender(
56.        address _market,
57.        address _lender,
58.        uint256 _forwardTimestampLimit
59.    ) private {
60.        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
61. 
62.        uint256 lastUserUpdateEpoch = lendingMarketBalancesEpoch[_market][_lender];
63.        uint256 updateUntilEpoch = Math.min(currEpoch, _forwardTimestampLimit);

The comment at L54 shows that the developer intends to use higher value in relation to _forwardTimestampLimit variable when needed but the opposite of the application of this intention was done at L63 as Math.min() function was used which selects the minimum value of the two variable. This Error can be spotted at two other instances at L82-L86 & L116-L123 of the LendingLedger.sol contract

Tools Used

Solidity, Manual Review

Recommended Mitigation Steps

Math.max should be used if that is the intention of the developer, if not the comment should be corrected to avoid misleading functionality.

Assessed type

Error