veRWA - Yuki's results

Lines of code

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

Vulnerability details

Impact

Lock owner won't be able to undelegate back to himself after his lock has expired, as a result his lock will be permanently stuck in the system and he won't be able to withdraw his funds back.

Proof of Concept

In order for a user to withdraw his lock, the lock will need to pass three important requirements:

• locked amount should be above zero indicating that the user has lock
• the current block.timestamp should be bigger than the lock end indicating that the lock has expired
• user should be his own delegatee indicating that the lock wasn't delegated to someone else

    function withdraw() external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.end <= block.timestamp, "Lock not expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");
        // Update lock
        uint256 amountToSend = uint256(uint128(locked_.amount));
        LockedBalance memory newLocked = _copyLock(locked_);
        newLocked.amount = 0;
        newLocked.end = 0;
        newLocked.delegated -= int128(int256(amountToSend));
        newLocked.delegatee = address(0);
        locked[msg.sender] = newLocked;
        newLocked.delegated = 0;
        // oldLocked can have either expired <= timestamp or zero end
        // currentLock has only 0 end
        // Both can have >= 0 amount
        _checkpoint(msg.sender, locked_, newLocked);
        // Send back deposited tokens
        (bool success, ) = msg.sender.call{value: amountToSend}("");
        require(success, "Failed to send CANTO");
        emit Withdraw(msg.sender, amountToSend, LockAction.WITHDRAW, block.timestamp);
    }

The third require statement in the function withdraw enforces a user to undelegate his lock in order to withdraw, but currently the system doesn't predict a scenario when a delegated lock expires.

The following issue will occur:

1. Delegated lock expires.
2. The owner of the lock tries to withdraw, but without success as the lock needs to be undelegated first.
3. Owner tries to call the function delegate in order to undelegate the lock back to himself.
4. In the function delegate, when a user tries to delegate back to himself, the else if statement will be triggered which transfers the current delegatee back to the owner of the lock. In the end the function will revert as the second require statement will be triggered toLocked.end > block.timestamp indicating that the owner's lock has expired.

    function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.delegatee != _addr, "Already delegated");
        // Update locks
        int128 value = locked_.amount;
        address delegatee = locked_.delegatee;
        LockedBalance memory fromLocked;
        LockedBalance memory toLocked;
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) {
            // Delegate
            fromLocked = locked_;
            toLocked = locked[_addr];
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;
        } else {
            // Re-delegate
            fromLocked = locked[delegatee];
            toLocked = locked[_addr];
            // Update owner lock if not involved in delegation
            locked[msg.sender] = locked_;
        }
        require(toLocked.amount > 0, "Delegatee has no lock");
        require(toLocked.end > block.timestamp, "Delegatee lock expired");
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, toLocked, value, LockAction.DELEGATE);
    }

Expired delegated locks will be permanently stuck in the system, as neither the lock owner or the delegatee will be able to withdraw the funds:

• Owner of expired lock can't undelegate in order to withdraw.
• Delegatee can't withdraw the delegated funds to him, as the system keeps the delegated funds from a third party in the delegatee's LockedBalance even after delegatee withdraws his lock.

        uint256 amountToSend = uint256(uint128(locked_.amount));
        LockedBalance memory newLocked = _copyLock(locked_);
        newLocked.amount = 0;
        newLocked.end = 0;
        newLocked.delegated -= int128(int256(amountToSend));
        newLocked.delegatee = address(0);
        locked[msg.sender] = newLocked;
        newLocked.delegated = 0;

Tools Used

Manual review

Recommended Mitigation Steps

Enforce the require statements to be passed only if the inputted _addr is not the msg.sender. this indicates that the owner of the lock will either delegate his lock to someone else or the current delegatee will be transferred to another delegatee.

    function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.delegatee != _addr, "Already delegated");
        // Update locks
        int128 value = locked_.amount;
        address delegatee = locked_.delegatee;
        LockedBalance memory fromLocked;
        LockedBalance memory toLocked;
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) {
            // Delegate
            fromLocked = locked_;
            toLocked = locked[_addr];
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;
        } else {
            // Re-delegate
            fromLocked = locked[delegatee];
            toLocked = locked[_addr];
            // Update owner lock if not involved in delegation
            locked[msg.sender] = locked_;
        }
  
        if (_addr != msg.sender) {
             require(toLocked.amount > 0, "Delegatee has no lock");
             require(toLocked.end > block.timestamp, "Delegatee lock expired");
             require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        }
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, toLocked, value, LockAction.DELEGATE);
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Undelegation logic doesn't work as expected, duo to that an owner of a lock would be enforced to reset his lock time for another 5 years through the function increaseAmount in order to successfully undelegate.

Proof of Concept

There are few differences between the Voting Escrow of fiat dao and the fork of it made in veRWA.

• in fiat dao a lock owner is able to increase their lock amount without resetting the lock time, also there is a function only intended for increasing the unlock time of a lock.
• on the other hand the voting escrow in veRWA has only the function to increase the amount of the lock, which also resets the unlock time for another 5 year based on the current block.timestamp

Duo to this changes in order to undelegate in veRWA, the owner of the lock would need to extend his unlock time for another 5 years, which can be really frustrating if the lock owner wants to undelegate right before the lock end time in order to be able to withdraw.

Explanation of the issue:

1. Owner of a lock calls the function delegate. In the function delegate the if statement will be triggered which indicates that the owner is going to delegate to someone else. Right before the delegation happens, the function ensures that the delegatee's lock end time is longer than the lock end time of the owner.
2. Time passes and before the owner's lock end time expires, the owner calls the function delegate in order to undelegate himself. In the function delegate the else if statement will be triggered which transfers the delegate back to the owner. Before undelegation happens the function enforces that the owner's lock end time is bigger than the end time of the delegatee's lock.
3. This is a problem considering that when the delegation happened, it was the opposite and the delegatee was enforced to have a bigger lock end time than the owner's lock.
4. In order to undelegate the owner would need to extend his lock time to bigger than the delegatee. But the only option for that is to call the function increaseAmount which will reset his lock end time for another 5 years.

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

In fiat dao, lock owner is able to call the function increaseUnlockTime and set their lock time just to be over the delegatee's one in order to undelegate. However as there is no such function in veRWA, the only option for a lock owner to undelegate would be to call the function increaseAmount and reset their unlock time for another 5 years.

    function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.delegatee != _addr, "Already delegated");
        // Update locks
        int128 value = locked_.amount;
        address delegatee = locked_.delegatee;
        LockedBalance memory fromLocked;
        LockedBalance memory toLocked;
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) {
            // Delegate
            fromLocked = locked_;
            toLocked = locked[_addr];
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;
        } else {
            // Re-delegate
            fromLocked = locked[delegatee];
            toLocked = locked[_addr];
            // Update owner lock if not involved in delegation
            locked[msg.sender] = locked_;
        }
        require(toLocked.amount > 0, "Delegatee has no lock");
        require(toLocked.end > block.timestamp, "Delegatee lock expired");
        require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, toLocked, value, LockAction.DELEGATE);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

// Same fix as on my other issue

Enforce the require statements to be passed only if the inputted _addr is not the msg.sender. this indicates that the owner of the lock will either delegate his lock to someone else or the current delegatee will be transferred to another delegatee.

    function delegate(address _addr) external nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.delegatee != _addr, "Already delegated");
        // Update locks
        int128 value = locked_.amount;
        address delegatee = locked_.delegatee;
        LockedBalance memory fromLocked;
        LockedBalance memory toLocked;
        locked_.delegatee = _addr;
        if (delegatee == msg.sender) {
            // Delegate
            fromLocked = locked_;
            toLocked = locked[_addr];
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;
        } else {
            // Re-delegate
            fromLocked = locked[delegatee];
            toLocked = locked[_addr];
            // Update owner lock if not involved in delegation
            locked[msg.sender] = locked_;
        }
  
        if (_addr != msg.sender) {
             require(toLocked.amount > 0, "Delegatee has no lock");
             require(toLocked.end > block.timestamp, "Delegatee lock expired");
             require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
        }
        _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
        _delegate(_addr, toLocked, value, LockAction.DELEGATE);
    }

Assessed type

Error