cryptic - `VotingEscrow` unsafe downcasting from `uint256` to `int128` may cause some users' tokens to be permanently locked and never have voting power

[sherlock-admin4]

cryptic

Medium

VotingEscrow unsafe downcasting from uint256 to int128 may cause some users' tokens to be permanently locked and never have voting power

Summary

Creating a lock by depositing LP tokens in VotingEscrow allows users to receive a veNFT which gives them voting power for gauges.

Due to unsafe downcast when depositing, some users may permanently lose LP tokens that can never be recovered, in addition to never being able to vote on gauges for rewards.

Vulnerability Detail

The following function is executed whenever locks are initiated:

VotingEscrow.sol#L777-L810

    function _deposit_for(
        uint _tokenId,
@>      uint _value,
        uint unlock_time,
        LockedBalance memory locked_balance,
        DepositType deposit_type
    ) internal {
        LockedBalance memory _locked = locked_balance;
        uint supply_before = supply;

        supply = supply_before + _value;
        LockedBalance memory old_locked;
        (old_locked.amount, old_locked.end) = (_locked.amount, _locked.end);
        // Adding to existing lock, or if a lock is expired - creating a new one
@>      _locked.amount += int128(int256(_value));
        if (unlock_time != 0) {
            _locked.end = unlock_time;
        }
        locked[_tokenId] = _locked;

        // Possibilities:
        // Both old_locked.end could be current or expired (>/< block.timestamp)
        // value == 0 (extend lock) or value > 0 (add to lock or extend lock)
        // _locked.end > block.timestamp (always)
        _checkpoint(_tokenId, old_locked, _locked);

        address from = msg.sender;
        if (_value != 0 && deposit_type != DepositType.MERGE_TYPE && deposit_type != DepositType.SPLIT_TYPE) {
@>          assert(IERC20(lpToken).transferFrom(from, address(this), _value));
        }

        emit Deposit(from, _tokenId, _value, _locked.end, deposit_type, block.timestamp);
        emit Supply(supply_before, supply_before + _value);
    }

_value sent by the user is downcasted from uint256 -> int256 -> int128. Note that type(uint256).max > type(int256).max and type(uint128).max > type(int128).max.

We can see that if _value is greater than type(int128).max, the value will overflow when downcasted to int128, and is truncated by wrapping around to a negative value. The locked.amount stored will not reflect the actual _value sent by the user, and will instead store a negative value for locked.amount.

Let's look at what happens for the following scenarios:

1. User attempts to withdraw funds after lock time passes from depositing value greater than type(int128).max
2. User attempts to vote on a gauge using the veNFT that was minted by locking these funds

Scenario 1: Let's look at what happens when the user attempts to withdraw after their lock time has passed. Recall that locked.amount (type int128) is storing a negative value due to the overflow from deposit.

VotingEscrow.sol#L955-L979

    function withdraw(uint _tokenId) external nonreentrant {
        assert(_isApprovedOrOwner(msg.sender, _tokenId));
        require(attachments[_tokenId] == 0 && !voted[_tokenId], "attached");

        LockedBalance memory _locked = locked[_tokenId];
        require(block.timestamp >= _locked.end, "The lock didn't expire");
@>      uint value = uint(int256(_locked.amount));

        locked[_tokenId] = LockedBalance(0,0);
        uint supply_before = supply;
        supply = supply_before - value;

        // old_locked can have either expired <= timestamp or zero end
        // _locked has only 0 end
        // Both can have >= 0 amount
        _checkpoint(_tokenId, _locked, LockedBalance(0,0));

@>      assert(IERC20(lpToken).transfer(msg.sender, value));

        // Burn the NFT
        _burn(_tokenId);

        emit Withdraw(msg.sender, _tokenId, value, block.timestamp);
        emit Supply(supply_before, supply_before - value);
    }

_locked.amount (which is negative), is casted to int256 and then uint256. When it is casted to int256, it will remain negative, but when casted to uint256, it will underflow to type(uint256).max. The function will attempt to transfer type(uint256).max number of tokens to the user instead of the original amount locked which was slightly greater than type(int128).max. It is very unlikely that the contract will have this many tokens (if it does, than the user will be able to steal them all), causing permanent DoS of withdrawal. The user will never be able to get back their locked tokens.

Scenario 2: Let's look at what happens when the user attempts to vote on gauges after locking (recall locked.amount holds a negative value)

Voter.sol#L249-L285

    function _vote(uint _tokenId, address[] memory _poolVote, uint256[] memory _weights) internal {
        _reset(_tokenId);
        uint _poolCnt = _poolVote.length;
@>      uint256 _weight = IVotingEscrow(_ve).balanceOfNFT(_tokenId);

        ...

            if (isGauge[_gauge]) {
                require(isAlive[_gauge], "gauge already dead");
@>              uint256 _poolWeight = _weights[i] * _weight / _totalVoteWeight;
                require(votes[_tokenId][_pool] == 0);
@>              require(_poolWeight != 0);
                _updateFor(_gauge);

               ...
    }

VotingEscrow::balanceOfNFT returns 0 if amount locked is negative:

VotingEscrow.sol#L1017-L1028

    function _balanceOfNFT(uint _tokenId, uint _t) internal view returns (uint) {
        uint _epoch = user_point_epoch[_tokenId];
        if (_epoch == 0) {
            return 0;
        } else {
            Point memory last_point = user_point_history[_tokenId][_epoch];
            last_point.bias -= last_point.slope * int128(int256(_t) - int256(last_point.ts));
@>          if (last_point.bias < 0) {
                last_point.bias = 0;
            }
@>          return uint(int256(last_point.bias));
        }
    }

Therefore the poolWeight will be 0 and due to the require(_poolWeight != 0); statement, the voting call will revert.

Thus, all withdrawals and voting is blocked if the user locks a value of more than type(int128).max, causing permanent loss of funds.

Note:

Although it is unlikely someone will hold LP tokens greater than type(int128).max (decimal: 170141183460469231731687303715884105727), if it does happen the impact is huge, as it will cause the user to lose all funds permanently and never be able to vote using the veNFT that was minted.

Proof of Concept

Coded PoC

Add the following to `test/Voter.t.sol` and run `forge test --mt testLockedFunds -vv` ```javascript function testLockedFunds() public { // set gauge, rewards, external bribe voter.createGauge(address(pair), 0); address gaugeAddress = voter.gauges(address(pair)); address[] memory rewards = new address[](2); rewards[0] = address(USDC); rewards[1] = address(FRAX); ExternalBribe newExternalBribe = new ExternalBribe( address(voter), rewards ); vm.expectEmit(true, true, false, true); emit ExternalBribeSet(address(this), gaugeAddress, address(newExternalBribe)); voter.setExternalBribeFor(gaugeAddress, address(newExternalBribe)); Router.route[] memory routes = new Router.route[](1); routes[0] = Router.route(address(USDC), address(FRAX), true); assertEq( router.getAmountsOut(USDC_1, routes)[1], pair.getAmountOut(USDC_1, address(USDC)) ); uint256[] memory assertedOutput = router.getAmountsOut(USDC_1, routes); USDC.approve(address(router), USDC_1); router.swapExactTokensForTokens( USDC_1, assertedOutput[1], routes, address(owner), block.timestamp ); vm.startPrank(address(owner)); uint256 lockDuration = 5 * 7 * 24 * 3600; // 5 weeks FLOW.mint(address(owner), 1e50); DAI.mint(address(owner), 1e50); FLOW.approve(address(router), 1e50); DAI.approve(address(router), 1e50); router.addLiquidity(address(FLOW), address(DAI), false, 1e50, 1e50, 0, 0, address(owner), block.timestamp); // unrealistic value but allows for simplicity when testing uint256 lpAmount = flowDaiPair.balanceOf(address(owner)); console.log("Total amount LP before lock: ", lpAmount); flowDaiPair.approve(address(escrow), lpAmount); escrow.create_lock(lpAmount, lockDuration); vm.roll(block.number + 1); // fwd 1 block because escrow.balanceOfNFT() returns 0 in same block int128 amountLocked; (amountLocked, ) = escrow.locked(1); console.log("Amount locked: ", uint256(int256(amountLocked))); console.log("NFT balance: ", escrow.balanceOfNFT(1)); uint256 lpAmountAfter = flowDaiPair.balanceOf(address(owner)); console.log("Total amount LP after lock: ", lpAmountAfter); vm.stopPrank(); vm.warp(block.timestamp + 1 weeks); // set values for voting address[] memory pools = new address[](1); pools[0] = address(pair); uint256[] memory weights = new uint256[](1); weights[0] = 5000; vm.expectRevert(); voter.vote(1, pools, weights); vm.warp(block.timestamp + 4 weeks); // unlock time vm.expectRevert(); escrow.withdraw(1); } ```

Console Output

Running 1 test for test/Voter.t.sol:VoterTest
[PASS] testLockedFunds() (gas: 7812561)
Logs:
  Total amount LP before lock:  100000000000000000000000000000301999999999999999000
  Amount locked:  115792089237316195423570985008687907853124301955207669823926529107935559679000
  NFT balance:  0
  Total amount LP after lock:  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.11ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

These values allow for simple testing. In the test, the user attempts to lock 100000000000000000000000000000301999999999999999000 LP tokens but it is downcasted to int128 which will be negative due to overflow, so it underflows to type(uint256).max when casted back to uint256, causing an incorrect value for amount locked. User can never withdraw nor vote on gauges.

Impact

Permanent loss of funds, denial of service, no voting power for amount deposited, tokens stuck forever in VotingEscrow.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L777-L810

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L955-L979

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/Voter.sol#L249-L285

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L1017-L1028

Tool used

Manual Review

Recommendation

Use SafeCastLibrary for casting as it's done on Velodrome V2

Duplicate of #225