USER CAN'T UNDELEGATE FROM BLOCKED CONTRACT OR A QUITED LOCK

[code423n4]

Lines of code

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

Vulnerability details

Impact

The potential impacts of this issue are :

• unused voting power if user delegate to a contract that get blocked or to delegatee that quits his lock.
• user can be forced to increase Lock time for a long period in order to undelegate from a malicious contract that increased his lock period before getting blocked (the same for a user that increased his lock period before quiting), in this case the user's tokens are stuck in the contract and the only way to get them back is to quit and pay a big penalty

Proof of Concept

Let's consider the following :

Bob has locked for 3 months Alice has locked for 3 months Charlie has locked for 6 months contract1 has locked for 8 months

There 2 scenarios :

Scenario 1 :

Bob and Alice both delegates to contract1 After 1 month period contract1 suddenly increase his lock time by 1 year so the new contract1 lock time is 20 months. The manager notice that contract1 is dangerous and block him. Now Bob and Alice voting power is lost , in order to get is back they must increase their lock time to 20 months Instead of locking for 8 months, Bob and Alice are now stuck for 20 month Unless they want to quit their locks but the paid penalty whould be too big and unfair

Scenario 2 : (the same as the first but instead of contract1 it's another user)

Bob and Alice both delegates to Charlie After 1 month period Charlie suddenly increase his lock time by 1 year so the new Charlie lock time is 18 months. Charile quits his lock for some reason. Now Bob and Alice voting power is lost , in order to get is back they must increase their lock time to 18 months Instead of locking for 8 months, Bob and Alice are now stuck for 18 month Unless they want to quit their locks but the paid penalty whould be too big and unfair

Tools Used

Manual audit

Recommended Mitigation Steps

ALLOW USERS TO UNDELEGATE FROM BLOCKED CONTRACTS OR QUITED LOCK without having to increase their lock time which that doesn't go against the logic of the VotingEscrow

The following changes must be made to the delegate function :

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];
            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);
        } else if (_addr == msg.sender) {
            // Undelegate
            fromLocked = locked[delegatee];
            toLocked = locked_;

            /*  @audit comment
                  allow user to undelegate from blocked contract oa a quited lock
            */
            if (IBlocklist(blocklist).isBlocked(delegatee) || fromLocked.amount == 0 ){
                _delegate(delegatee, fromLocked, value, LockAction.UNDELEGATE);
                _delegate(_addr, toLocked, value, LockAction.DELEGATE);
            } else {
                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);
        }
        } 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);
        }

    }

[lacoop6tu]

This is an expected behaviour

[lacoop6tu]

Duplicate of #188

[lacoop6tu]

Duplicate of #188