FIAT DAO veFDT contest - 0xmatt's results

Vulnerability Details

Low/Non-Critical Findings

L-01 VotingEscrow.sol#transferOwnership() lacks two-step procedure and/or input validation routines for critical operations.

The transferOwnership() function in VotingEscrow.sol immediately transfers ownership of the voting contract to a new owner. According to the @dev comment, the owner should always be a timelock contract. This is not verified. Nor are other checks made.

If, for any reason the transferOwnership() function's _addr value is incorrect, a call would result in the immediate loss of access to the following VotingEscrow.sol functions:

1. transferOwnership()
2. updateBlocklist()
3. updatePenaltyRecipient()
4. unlock()

The issue's two components are:

1. Missing checks may lead to problems should something go wrong.
2. The immediate transfer means that access is lost at the point something goes wrong.

Currently the only check used is on L140:

require(msg.sender == owner, "Only owner");

The function checks that the caller is the owner via a require() check. However, it does not check that:

• The destination address exists
• That it is valid (ie not address(0))
• That the new owner is a timelock contract

Provided msg.sender matches the existing owner, the owner value is immediately updated to a caller-controlled address at Line 141.

If the _addr value doesn't match unverified expectations access to owner-restricted functions would be immediately and irrevocably lost at this point.

Implementing checks to ensure that the value submitted was not address(0) will ensure that if there's a problem with the existing owner contract's state, this won't cause ownership to be irrevocably transferred to a null address.

The code used in Blocklist.sol can be modified as a check to ensure that the new address is at least a contract:

        uint256 size;
        assembly {
            size := extcodesize(_addr)
        }
        require(size > 0, "Not a contract");

As well as checking the contract's codesize to confirm that it is in fact a contract the check will also fail for address(0) (as it has no code).

It's difficult to verify that the new contract is a timelock contract. However, a two-step transfer + claim with timelock pattern could work. Using this pattern, the new contract would propose a change of ownership. The existing owner would then verify the contract as above and initiate a timelock, emitting a message that a change is in progress.

At this point the existing manager would review the values and if incorrect, cancel the change. Ownership will only be transferred from the original contract once the timelock expires, providing it hasn't been cancelled.

L-02 Wraparound issue in createLock()

The createLock() function in VotingEscrow.sol takes a user-supplied uint256 _value variable, and recasts it several times on lines 418 and 420. Because the uint256 is recast to an int128, large uint256 values will result in negative int128 values.

For example, submitting a value > 170141183460469231731687303715884105727 will result in locked.amount and locked_.delegated values of -170141183460469231731687303715884105728.

Even stranger, submitting a value of 57896044618658097711785492504343953926634992332820282019728792003956564819968 will be recast as int128(0). If someone submitted this value of an ERC20 token and the transferFrom succeeded, their locked_.amount and locked_.delegated values would be 0. Because of this the locked_.amount == 0 test at Line 413 would pass if they called the function again, but their existing balance would be lost.

Whether this is a risk depends on what kinds of ERC20 tokens and emissions are used. If the VotingEscrow contract was opened up to non-native ERC20 tokens, it's possible that someone could supply this value to create a lock and end up with a resulting locked and delegated value of 0.

Other instances

Version of this also happen on lines 460, 461, 465, 472 of VotingEscrow.sol in the increaseAmount() function. This has the unwelcome effect of potentially turning values negative which would result in the requires on lines 449 and 469 failing, effectively losing their lock.

L-03 Lock Delegation Not Enforced

The contest documentation has this to say about Lock Delegation:

the delegatee's lock expiration needs to be longer than the delegator's.

The contract does not enforce this, with potential consequences for users:

1. The contract allows two parties to create locks with the same _unlockTime and delegate to each other.
2. Even if bob's unlock is longer than alice's, alice can delegate then extend her lock past Bob's.

To address points 1 and 2:

1. If the lock expiration needs to be longer than the delegators, then replace the '>=' in L589 with a '>'.
2. In increaseUnlockTime() (L493-L523) check the value of locked[msg.sender].delegatee, then revert if the requested increase is >= the delegatee's end.

L-04 No Way To Remove People From Blocklist

A manager can call Blocklist.sol#block() to block addresses. There is no way to unblock a mistake. If the manager makes a mistake, or accidentally misgenerates an address, addresses will be permanently blocked.

1. Add a check to make sure addr !=0
2. Add an unblock() method to allow managers to unblock.

L-05 Blocklist Migration Requires Separate List Of Addresses

The blocklist is stored as private in Blocklist.sol while the isBlocked() function provides a public way to check if an individual address is blocked. The function VotingEscrow.sol#updateBlocklist enables the VotingEscrow.sol owner to migrate to a new blocklist contract.

There is no way to obtain a full list of existing blocked addresses from Blocklist.sol. This means that a separate list of blocked addresses needs to be maintained so that it can be restored in the event of a blocklist upgrade/address-change.

1. Create a function that returns the whole of blocklist for migrations.
2. Consider implementing a migrateFrom() function in Blocklist.sol that given a list of addresses will add it to a blocklist.

Both of these functions should be restricted to a manager role.

L-06 No Way To Update Manager Contract Address

Blocklist.sol sets the manager and ve contract addresses in the constructor. However there is no way to update the manager address once deployed.

If the manager address needs to change (e.g. keys are lost, or an upgrade to a manager contract) a new blocklist contract needs to be deployed.

1. Create an updateManager() function that allows a manager or owner to change the manager, ideally following the two-step procedure pattern.

L-07 Blocklist Can't Support Ve Contracts

The ve address only allows the blocklist to manage one ve contract. Normally this wouldn't be a problem but in scenarios where multiple ve contracts exist, separate blocklists will be required for each.

For example, in a major ve contract upgrade people would still be timelocked in the old contract as no migration mechanism exists. This would require two separately managed blocklist contracts, which could easily end up desynchronized. This is because of two reasons:

1. The ve address is set at construction and cannot be changed.
2. The ve address is singular and the forceUndelegate() call has to call a specific ve instance.

Moving ve to an array of addresses and creating add and remove function accessible by the manager role will provide blocklist support for multiple ve contracts without the need to migrate.