FIAT DAO veFDT contest - scaraven's results

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L426 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L546 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L652-L657

Vulnerability details

Impact

Fee on transfer tokens are ERC20 tokens which charge a small fee for every transfer between accounts. This means that the amount of tokens sent and the amount received are not the same. However, fiatDAO does assume that received and sent amounts are the same, which causes a disparity between how many tokens the contract thinks it contains and how much actually does.

This can prevent a user from withdrawing from the contract due to an insufficient balance and therefore locking the user's funds into the contract

Proof of Concept

1. Alice locks 500 USDC into VoteEscrow.sol, however due to the fee, only 499 is received by the contract
2. The contract sets Alice's lock amount to 500
3. After Alice's lock expires, she calls withdraw() and the contract attempts to send 500 USDC to Alice
4. The transaction reverts due to insufficient balance in the contract and Alice's funds are stuck in the contract

Tools Used

VS Code

Recommended Mitigation Steps

Consider modifying createLock() to:

        ...
        uint256 balanceBefore = token.balanceOf(address(this));
        locked_.end = unlock_time;
        locked_.delegatee = msg.sender;
        locked[msg.sender] = locked_;
        // Deposit locked tokens
        require(
            token.transferFrom(msg.sender, address(this), _value),
            "Transfer failed"
        );
        uint256 amount = token.balanceOf(address(this)) - balanceBefore;
        locked_.amount += int128(int256(amount));
        locked_.delegated += int128(int256(amount));
        _checkpoint(msg.sender, LockedBalance(0, 0, 0, address(0)), locked_);
       ...

Note, that this breaks the CEI pattern, however the use of nonReentrant will prevent Reentrancy.

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/features/Blocklist.sol#L25

Vulnerability details

Impact

On the contest page, it is written that:

SmartWallets (i.e. contracts) can create a lock without being approved first. However, the veFDT owner maintains a Blocklist where SmartWallets may be blocked from further interacting with the system.

However, a user who owns a smart wallet can prevent their wallet from being blocked by selfdestructing the smart wallet so that the bytecode is empty. This effectively disguises the address as an EOA and prevents the smart wallet from ever being blocked. Using create2, which generates an address solely on the contract byte code, deployer address and a salt, the user can redeploy the same contract code at the same address and carry on using it as normal.

I have opted for medium severity because it invalidates a major feature of the protocol but does not impact any other users

Proof of Concept

1. Blocklist manager tries to block smart wallet A with block()
2. Alice, the owner of smart wallet A, sees the transaction in the mempool and frontruns the transaction where she calls a function in the smart wallet so that the wallet runs selfdestruct()
3. The wallet self-destructs and now has a bytecode of 0 so the _isContract(addr) check fails
4. The block() transaction reverts and Alice can redeploy the same smart wallet at the same address using create2

N.B. that alice does not even need to frontrun the block() transaction. For a higher gas use, alice can deploy the contract byte code with create2, make any arbitrary call to the smart wallet and then selfdestruct it at the end of the transaction. This would mean that the smart wallet will have 0 bytecode outside alice's transaction and makes it impossible for an admin to block the wallet's address

Tools Used

VS Code

Recommended Mitigation Steps

There are two possible mitigations:

1. Use private transactions to prevent frontrunning however, as outlined at the end of the POC, this does not completely solve the problem
2. Remove the _isContract check. I do not see a problem in being able to block EOA's

Lines of code

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

Vulnerability details

Impact

The original function for increaseUnlockTime() in Curve's Vyper code is:

def increase_unlock_time(_unlock_time: uint256):
    """
    @notice Extend the unlock time for `msg.sender` to `_unlock_time`
    @param _unlock_time New epoch time for unlocking
    """
    self.assert_not_contract(msg.sender)
    _locked: LockedBalance = self.locked[msg.sender]
    unlock_time: uint256 = (_unlock_time / WEEK) * WEEK  # Locktime is rounded down to weeks

    assert _locked.end > block.timestamp, "Lock expired"
    assert _locked.amount > 0, "Nothing is locked"
    assert unlock_time > _locked.end, "Can only increase lock duration"
    assert unlock_time <= block.timestamp + MAXTIME, "Voting lock can be 4 years max"

    self._deposit_for(msg.sender, 0, unlock_time, _locked, INCREASE_UNLOCK_TIME)

The original function makes sure that the lock has not already expired when increasing the end of the vesting period. However, in the case of fiatDAO, this check is only made if the delegatee is the same as the lock owner. Therefore, as long as delegatee != msg.sender, the lock owner can arbitrarily increase their lock time without any consequences. This negatively affects any delegators to this lock because they are not able to delegate to a different lock as long as the current delegation vesting period ends after the new one.

Proof of Concept

1. Alice creates a lock and delegates her votes to Bob
2. After a certain time period, Bob's lock expires
3. Bob increases his vesting period to the maximum amount with increaseUnlockTime()
4. Alice tries to delegate to herself, however, the delegation fails because Bob's lock now ends later
5. If Alice wants to delegate to herself, she must set her lock to end at the same time as Bob and then change her delegation Bob, however, has forced Alice to wait a very long period to withdraw or pay a penalty she shouldn't have

The impact of this issue is very similar to #74 (A delegator can grief their delegatee and prevent withdrawals) however the underlying issue is different as this issue worries about expired locks unfairly increasing their vesting period

Tools Used

VS Code

Recommended Mitigation Steps

In increaseUnlockTime(), move the expiry check outside the if statement

Lines of code

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

Vulnerability details

Impact

In increaseUnlockTime(), when locked.delegatee == msg.sender the contract creates a new variable oldLocked which is a copy of locked_ and then sets the end to the increased unlock_time. This means that oldLocked and locked_ are exactly the same, so when the contract calls _checkpoint(), no change is made to the user's vesting period.

This causes the vesting period of the user to increase without changing the voting weight decay properly. As this is a major logic error in VotingEscrow.sol, I believe high severity is suitable

Proof of Concept

1. Alice creates a new lock with createLock()
2. Some time later, Alice wishes to increase her lock time by calling increaseUnlockTime()
3. increaseUnlockTime() calls _checkpoint() where fromLocked is the same as toLocked
4. As userNewPoint.slope == userOldPoint.slope and userNewPoint.bias == userOldPoint.bias, no change is made to the voting schedule
5. Therefore, Alice needs to wait longer to withdraw with no increase in voting weight

Tools Used

VS Code

Recommended Mitigation Steps

Change

            oldLocked.end = unlock_time;

to

            oldLocked.end = oldUnlockTime;

Lines of code

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

Vulnerability details

Impact

When a user calls increaseUnlockTime(), if they have delegated to someone else, then the vesting period increases however no change is made to the voting weight of the user even if other users have delegated to that user.

This means that the user increases their lock time without gaining an increase in voting weight therefore I believe high severity is suitable

Proof of Concept

1. Alice creates a lock
2. Bob delegates his voting power to Alice while Alice delegates her locked funds to Charlie
3. Alice wants to increase her voting period time and calls increaseUnlockTime()
4. As Alice has delegated towards Charlie, the check locked_.delegatee == msg.sender fails so _checkpoint() is not called
5. Due to Bob's delegation to Alice, Alice should have an increase in voting weight however this does not occur

Tools Used

VS Code

Recommended Mitigation Steps

Either consider completely removing the if statement or modify it so that the if statement becomes

if(locked_.delegated > 0) {