Unsafe casting from int128 can cause wrong accounting of locked amounts and make user lose tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L300 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L276 https://github.com/code-423n4/2023-08-verwa/blob/a693b4db05b9e202816346a6f9cada94f28a2698/src/VotingEscrow.sol#L288-L323

Vulnerability details

Impact

The unsafe casting to int128 variable can cause its value to be different from the correct value. For example in the createLock function, the addition to the locked amount variable is done by locked_.amount += int128(int256(_value)). In that case, if _value is greater than type(int128).max which is 2**127 - 1, then the accounting will be wrong and the amount that will be added to locked_.amount will be less than the amount of token that will be transferred from the user. Then the user won’t be able to withdraw the tokens that he transferred, and they’ll be stuck in the contract forever.

Proof of Concept

This is a simple POC where a user deposit a high amount and withdraw it, but in the other test deposit a high amount plus one and the whole deposit gets locked. This issue could also happen using the increaseAmount method.

// SPDX-License-Identifier: unlicensed
pragma solidity ^0.8.16;

import "forge-std/Test.sol";
import "../VotingEscrow.sol";

contract VotingEscrowBugTest is Test {
    VotingEscrow public ve;

    address public user1 = makeAddr("user1");

    function setUp() public {
        ve = new VotingEscrow("Voting Escrow", "VE");
    }

    function _lockAndTest(uint256 amount) internal {
        deal(user1, amount);

        vm.prank(user1);
        ve.createLock{value: amount}(amount);

        ve.locked(user1);

        vm.warp(ve.LOCKTIME() + 1);
        vm.prank(user1);
        ve.withdraw();
    }

    function testCreateLockWithHighValue() public {
        uint256 amount = uint256(int256(type(int128).max));
        _lockAndTest(amount);
        assertEq(user1.balance, amount);
    }

    function testCreateLockWithHighValuePlusOne() public {
        // @audit this will let the user deposit but nerver withdraw
        uint256 amount = uint256(int256(type(int128).max))+1;
        _lockAndTest(amount);
        assertEq(user1.balance, amount);
    }
}

Tools Used

Manual revision

Recommended Mitigation Steps

Use SafeCast Or you could check that msg.value == int128(int256(_value))

Assessed type

Math

[c4-pre-sort]

141345 marked the issue as duplicate of #16

[alcueca]

Reported in the bot race

[c4-judge]

alcueca marked the issue as unsatisfactory: Out of scope