RE-DELEGATE AND UNDELEGATE CAN BE FRONT RUN IN VotinEscrow.sol

[code423n4]

Lines of code

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

Vulnerability details

Impact

User can not undelegate or redelegate is a malicius delegatee front run the user transaction increasing his amount of time to end calling increaseAmount.

Proof of Concept

We can explore this analyzing both functions:

file:src/VotingEscrow.sol
    function delegate(address _addr) external nonReentrant {
        
        ...
        } 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.end >= fromLocked.end, "Only delegate to longer lock");
        ...
    }

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

This function require that old delegatee end be less than the new delegatee.

Now analyse the increaseAmount function:

file:src/VotingEscrow.sol
function increaseAmount(uint256 _value) external payable nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        ...
        LockedBalance memory newLocked = _copyLock(locked_);
        newLocked.amount += int128(int256(_value));
        newLocked.end = _floorToWeek(block.timestamp + LOCKTIME);
        if (delegatee == msg.sender) {
            // Undelegated lock
            action = LockAction.INCREASE_AMOUNT_AND_DELEGATION;
            newLocked.delegated += int128(int256(_value));
            locked[msg.sender] = newLocked;
            _checkpoint(msg.sender, locked_, newLocked);
        } else {
            // Delegated lock, update sender's lock first
            locked[msg.sender] = newLocked;
           ...
        }
        emit Deposit(msg.sender, _value, unlockTime, action, block.timestamp);
    }

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

This function is increasing the time of the end by 5 years, so if a delegatee see in the mempool that he is gonna be undelegate or re delegate, he can front run that transaction calling increaseAmount passing 1 wei and increasing his end for 5 years more breaking the require(toLocked.end >= fromLocked.end, "Only delegate to longer lock") in delegate function.

Tools Used

manual

Recommended Mitigation Steps

One recomendation is follow what FIAT DAO is doing in his VotingEscrow contract,
other approach that the project can take is eliminate the increasing of the time end when user call increasAmount, there is not incentive to the user to call that function if he is gonna lock for 5 years more

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #116

[c4-pre-sort]

141345 marked the issue as duplicate of #82

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as partial-50

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as duplicate of #375

[c4-judge]

alcueca marked the issue as partial-50

[c4-judge]

alcueca marked the issue as duplicate of #182

[c4-judge]

alcueca changed the severity to 3 (High Risk)