FIAT DAO veFDT contest - DecorativePineapple's results

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L115-L116

Vulnerability details

Impact

In the VotingEscrow.sol constructor, the decimals are calculated based on the specified ERC20 tokens decimals. Then on the L:116 a require statement enforces that the max decimals of the ERC20 token is 18. This is not always the case, as some ERC20 tokens can have more than 18 decimals, such as the YAMv2 token, which has 24.

Proof of Concept

decimals = IERC20(_token).decimals();
require(decimals <= 18, "Exceeds max decimals");

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to remove the require statement from L:116 and adjust the calculations in a way that work with ERC20 decimals.

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L403

Vulnerability details

Impact

In the createLock function the amount is calculated by casting the uint256 _value to int128 in an unsafe way. Specifically the locked_.amount is calculated as: locked_.amount += int128(int256(_value)); could result in a negative value for locked._amount. For example, if _value is 868790785326998466564056403945758400791, the locked._amount would be -152056315435816923826067418349546233577

Because the locked._amount isn't checked whether it is >0 in the rest of the function, a lock can be created with negative amount. When the user that created that lock tries to withdraw the tokens (either with withdraw or quitLock) , he will be unable to withdraw the tokens.

This bug could happen if a user tries to create a lock with at least max(uint128) tokens. He has to approve at least that amount of tokens to the contract, so the transferFrom call from line L657 would succeed.

This is due to the check require(locked_.amount > 0, "No lock"); that is used in both functions.

  function withdraw() external override nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.end <= block.timestamp, "Lock not expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");
        // Update lock
        ...

and

function quitLock() external override nonReentrant {
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(locked_.end > block.timestamp, "Lock expired");
        require(locked_.delegatee == msg.sender, "Lock delegated");
        // Update lock
        ...

The createLock function:

 function createLock(uint256 _value, uint256 _unlockTime)
        external
        override
        nonReentrant
        checkBlocklist
    {
        uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
        LockedBalance memory locked_ = locked[msg.sender];
        // Validate inputs
        ...
        // Update lock and voting power (checkpoint)
        locked_.amount += int128(int256(_value));
        locked_.end = unlock_time;
        locked_.delegated += int128(int256(_value));
        locked_.delegatee = msg.sender;
        locked[msg.sender] = locked_;
        _checkpoint(msg.sender, LockedBalance(0, 0, 0, address(0)), locked_);
        // Deposit locked tokens
        require(
            token.transferFrom(msg.sender, address(this), _value),
            "Transfer failed"
        );
     ...
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Even though Solidity 0.8.x is used, type casts do not throw an error. A SafeCast library must be used everywhere a typecast is done. Because the locked._amount isn't intended to receive negative values, it is better to use uint128 for the variables amount and delegated of the LockedBalance struct, and adjust the typecasting accordingly.

No zero address checks

There is currently no input validation on the constructors of the VotingEscrow.sol and the constructor of the Blocklist.sol contracts. The variables manager , ve of the Blocklist.sol contract and the variables _owner, _penaltyRecipient and token of the VotingEscrow.sol contract have to be validated. Not validating against a zero address could potentially lead to an initialized state where the contracts have no owner and the deployer needs to re-deploy the contract to have it working properly.

Wrong comment in recordKeyPurchase

The comment on L:589 : require(toLocked.end >= fromLocked.end, "Only delegate to longer lock"); is wrong. In fact, it allows a delegate to both longer locks and lock that has the same end time.