FIAT DAO veFDT contest - carlitox477's results

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L403 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L418-L435 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L440 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L460-L462 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L471-L473 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L485-L488

Vulnerability details

Impact

Users can lose funds by sending a wrong _value parameter. The problems relays in casting _value to int128. Solidity does not check casting, just math operations. If a user tries to lock an amount greater than 2^128, the transaction won't be reverted and token will be transfered from the user to the contract

Proof of Concept

1. Bob wants to deposit 2^129 tokens, this means that the only bit set as 1 will be bit n° 130. Bob has no previous lock.
2. Bob calls function createLock with parameters 2^129 and _unlockTime=block.timestamp+MAXTIME/2. This means that the inputs are valid,
3. int128(int256(value)) cut the bit chain in two chains of 128 bits, and keeps the second one which is just full of zeros. However, solidity does not detect this. Consequently, locked.amount and locked_.delegated are set to zero, but token.transferFrom(msg.sender, address(this), _value) will succeed.
4. There is no way for Bob to recover his tokens, given that withdraw has the same issues with casting

A similar problem can be seen in function increaseAmount, and it is marked in the attached links

Recommended Mitigation Steps

Option 1

Change _value type to int128

Option 2

Use library SafeCast from openzeppelin, or just check that _value <= 2^128

Option 3

Change LockedBalance struct to

struct LockedBalance {
        uint256 amount;
        uint256 end;
        uint256 delegated;
        address delegatee;
    }

However, consider how this will affect other functions

MAXTIME Unnecesary double casting to int128

Giving 2^128 = 2^7 * 2^111 * 2^10, MAXTIME = 365 days = 60 * 60 * 24 * 365 = 2^7 * 5^3 * 3^3, and that 2^7 * 5^3 * 3^3 * 2^111 * 2^10, we can avoid double casting int128(int256(MAXTIME)) to just int128(MAXTIME)

int128(int256(_oldLocked.end - block.timestamp)) unnecessary double casting

Giving that _oldLocked.end - block.timestamp <= MAXTIME, and previous justification, we can simplify this to just int128(_oldLocked.end - block.timestamp)

Unnecesary sum operation in createLock function

It is required that locked_.amount == 0, meaning that instead of doing locked_.amount += int128(int256(_value)) and locked_.delegated += int128(int256(_value));, it would be better to just do an assignation:

• locked_.amount = int128(int256(_value))
• locked_.delegated = int128(int256(_value));