User can't move delegation to someone who have lesser unlock time.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L555-L592

Vulnerability details

Impact

User can't move delegation to someone who have lesser unlock time. It will be reverted with "Only delegate to longer lock"

Proof of Concept

    function delegate(address _addr)
        external
        override
        nonReentrant
        checkBlocklist
    {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");
        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);
    }

Let assume user and delegatee and another _addr has created lock with following unlock time

user: t + 500
delegatee: t + 1000
_addr: t + 800

When user call delegate(delegatee) it success and locked_.delegatee will change to delegatee

But once user call delegate(_addr) to move delegation to _addr it will fail. Observe that fromLocked and toLocked will have following unlock time on undelegation.

fromLocked: delegatee: t + 1000
toLocked: _addr : t + 800

Since toLocked.end = t + 800 < fromLocked.end = t + 1000 and require(toLocked.end >= fromLocked.end, "Only delegate to longer lock"), It will revert with "Only delegate to longer lock".

As a result, user can't move delegation to someone who have lesser unlock time.

In fact, it should be able to move since _addr has greater unlock time than user.

Recommended Mitigation Steps

Review check step in delegate function again to check between toLocked (_addr) and user. Currently, it check toLocked (_addr) and fromLocked (delegatee) which is wrong.

[lacoop6tu]

Duplicate of #188