FIAT DAO veFDT contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L425-L428 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L485-L488 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#L657 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L676

Vulnerability details

Impact

Some ERC20 tokens functions don't return a boolean, for example USDT, BNB, OMG. So the VotingEscrow contract simply won't work with tokens like that as the token.

Proof of Concept

The USDT's transfer and transferFrom functions doesn't return a bool, so the call to these functions will revert although the user has enough balance and the VotingEscrow contract won't work, assuming that token is USDT.

Tools Used

Manual auditing - VS Code, some hardhat tests and me :)

Recommended Mitigation Steps

Use the OpenZepplin's safeTransfer and safeTransferFrom functions

Lines of code

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

Vulnerability details

Impact

Some ERC20 tokens implemented so a fee is taken when transferring them, for example STA and PAXG. The current implementation of the VotingEscrow contract will mess up the accounting of the locked amounts if token will be a token like that, what will lead to a state where users won't be able to receive their funds.

This will happen because the value that is added to the locked amount is not the actual value received by the contract, but the value supplied by the user (the value which the fee is taken from).

Proof of Concept

The STA token burns 1% of the value provided to the transfer function, which means the recipient gets only 99% of the transferred asset. Let's assume that token is the address of the STA token.

1. Bob wants to lock 100 STA tokens and calls createLock(100 * 10**18, unlockTime).
2. The addition to the locked amount variable is done with 100 * 10**18, while the actual amount that was received by the contract is 99 * 10**18.
3. When the lock expires Bob will try to withdraw his tokens, and the transfer function will be called with the accounted locked amount (which is 100 * 10**18). This might succeed due to other users locking too, so the transferred tokens will be taken from "their tokens", but in the end there will be users left without an option to withdraw their funds, because the balance of the contract will be less than the locked amount that the contract is trying to transfer.

Tools Used

Manual auditing - VS Code and me :)

Recommended Mitigation Steps

Calculate the amount to add to the locked amount by the difference between the balances before and after the transfer instead of using the supplied value.

Lines of code

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

Vulnerability details

Impact

The increaseUnlockTime function sends a wrong unlock time to the _checkpoint function in the oldLocked variable - the locked_ variable (which is the new LockedBalance) is copied into the oldLocked variable (which is supposed to be the previous LockedBalance), but instead of assigning the previous unlock time to it, the new unlock time is assigned to it which basically leaves it like the new LockedBalance.

if (locked_.delegatee == msg.sender) {
    // Undelegated lock
    require(oldUnlockTime > block.timestamp, "Lock expired");
    LockedBalance memory oldLocked = _copyLock(locked_);
    oldLocked.end = unlock_time;
    _checkpoint(msg.sender, oldLocked, locked_);
}

That messes up the checkpoint accounting and should be fixed.

Proof of Concept

As I mentioned before, the bug can be seen in these lines of the increaseUnlockTime function:

if (locked_.delegatee == msg.sender) {
    // Undelegated lock
    require(oldUnlockTime > block.timestamp, "Lock expired");
    LockedBalance memory oldLocked = _copyLock(locked_);
    oldLocked.end = unlock_time;
    _checkpoint(msg.sender, oldLocked, locked_);
}

Here we can see that the end field of the oldLocked variable is set to the new unlock time instead of the old one.

I modified the test that checks the increaseUnlockTime to print the difference between the total supply and the user's balance, and it seems that the total supply after the call to increaseUnlockTime is less than the balance of the user.

The test:

it("allows user to extend lock", async () => {
    await goToNextUnixWeekStart();

    const bobSnapBefore = await snapshotData(bob);
    var balanceBefore = await votingLockup.balanceOf(bob.address);
    var totalSupplyBefore = await votingLockup.totalSupply();
    
    await votingLockup
    .connect(bob)
    .increaseUnlockTime(start.add(ONE_YEAR));
    
    const bobSnapAfter = await snapshotData(bob);
    var balanceAfter = await votingLockup.balanceOf(bob.address);
    var totalSupplyAfter = await votingLockup.totalSupply();

    console.log("total supply - balance of bob before: ");
    console.log(totalSupplyBefore.sub(balanceBefore));
    console.log("total supply - balance of bob after: ");
    console.log(totalSupplyAfter.sub(balanceAfter));
});

The output:

total supply - balance of bob before: 
BigNumber { _hex: '0x0e76231788418c10d3', _isBigNumber: true }
total supply - balance of bob after: 
BigNumber { _hex: '-0x0c91bf21e7fb2d2bda', _isBigNumber: true }

The output after correcting the value passed to the _checkpoint function:

total supply - balance of bob before: 
BigNumber { _hex: '0x0e76231788418c10d3', _isBigNumber: true }
total supply - balance of bob after: 
BigNumber { _hex: '0x0e7622f9d3c1222826', _isBigNumber: true }

Another thing I noticed here is the fact that the differences between the total supply and the user's balance are not equal also in the fixed version, but I couldn't understand why (I asked @elnilz and he said he'll check it)

Tools Used

Manual auditing - VS Code and me :)

Recommended Mitigation Steps

Replace oldLocked.end = unlock_time; on line 513 with oldLocked.end = oldUnlockTime;

Lines of code

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

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

1. Alice tries to lock 2**128 tokens. She calls createLock(2**128, unlockTime) with the time she wants to lock for.
2. The addition of the given value is done by locked_.amount += int128(int256(_value)), which actually does nothing to the locked_.amount variable and it remains 0. That's because when casting int128(int256(2**128)) truncates to 0, and that leaves the locked amount unchanged but the tokens are transferred.

Tools Used

Manual auditing - VS Code and me :)

Recommended Mitigation Steps

Make sure that the values fit in the variables you are trying to assign them to when casting variables to smaller types.

Lines of code

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

Vulnerability details

Impact

The virtual balance of a user is calculated using 2 values - the amount that is delegated to that user, and his lock period. When calling the increaseUnlockTime function, we want to checkpoint the user's data as long as he doesn't have any funds. This is because if he has funds, the change to the lock time will effect his virtual balance.

The code checks that the user doesn't have any funds by checking that he is not delegating his funds to someone else. This check can be seen here:

if (locked_.delegatee == msg.sender) {
    // Undelegated lock
    require(oldUnlockTime > block.timestamp, "Lock expired");
    LockedBalance memory oldLocked = _copyLock(locked_);
    oldLocked.end = unlock_time;
    // oldLocked.end = oldUnlockTime;
    _checkpoint(msg.sender, oldLocked, locked_);
}

However, this check is wrong, because a user can delegate his funds but still has funds delegated to him, so the change to the unlock time will effect his virtual balance and is needed to be accounted by the _checkpoint function.

Proof of Concept

1. Alice locked X tokens and delegated them to Bob, so locked[alice].delegatee == bob
2. Bob locked Y tokens and delegated them to Charlie, so locked[bob].delegatee == charlie Now:
   • locked[alice].delegated is 0 because all of Alice's funds are delegated to Bob and she doesn't have any funds delegated to her.
   • locked[bob].delegated is X because all of Bob's funds are delegated to Charlie, but Bob has Alice's funds delegated to him.
   • locked[charlie].delegated is Y because Charlie doesn't have any funds locked, but he has Bob's funds delegated to him.
3. Bob wants to increase the unlock time of his locked funds. He calls increaseUnlockTime with the new unlock time he wants. In that call, the virtual balance of Bob is changed, because although he delegated his funds to Charlie, he still has Alice's funds delegated to him and the change to the unlock time will effect it. However, because of the wrong condition shown before, the _checkpoint function won't be called and his virtual balance won't be updated - locked[bob].delegatee == charlie.

Tools Used

Manual auditing - VS Code and me :)

Recommended Mitigation Steps

The correct condition here needs to check whether the user has ANY funds in the system, not if the user has delegated his funds or not. This can be checked by this condition:

if (locked_.delegated > 0) {
    // Undelegated lock
    require(oldUnlockTime > block.timestamp, "Lock expired");
    LockedBalance memory oldLocked = _copyLock(locked_);
    oldLocked.end = unlock_time;
    // oldLocked.end = oldUnlockTime;
    _checkpoint(msg.sender, oldLocked, locked_);
}

Lines of code

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

Vulnerability details

Impact

An attacker can use a flashloan and the quitLock function to achieve a large amount of votes for one transaction. It can, depends on the implementation of the modules that will use this contract, be used to pass malicious proposals or exploit any feature that depends on the voting balance.

Proof of Concept

We assume here that there is a contract that provides a flashloan (for simplicity without fees, but can also work with fees, just requires the attacker to provide a larger amount of tokens) for the token that is used by the VotingEscrow contract.

1. The attacker deploys a smart contract that implements the following logic and approves the contract for the token that is used in the VotingEscrow contract (of course assigning all the values of the variables).
2. The attacker calls the attack function with the amount he wants to take as a flashloan (he will need to cover a penalty based on that amount).
3. The attack function calculates the penalty and transfers it from the attacker.
4. The flashloan function is called, which provides the tokens to the contract and then calls the flashloanCallback with the lent amount.
5. The flashloanCallback function creates a lock with the amount received from the flashloan for a week (the unlock time can be changed to achieve larger votes balance, but it must be considered when calculating the penalty).
6. The attacker can do whatever he wants with the amount of votes the contract currently has.
7. The quitLock function is called to get back the funds (excluding the penalty), and the loan is payed back.

contract VotingEscrowAttack {
    IERC20 constant token = IERC20(0x...); // the token used in the VotingEscrow contract
    IVotingEscrow constant votingEscrow = IVotingEscrow(0x...); // the address of the VotingEscrow contract

    uint constant WEEK = 1 weeks;
    uint constant MAXTIME = 365 days;
    uint constant MAXPENALTY = 10**18;

    uint constant PENALTY_RATE = (WEEK * MAXPENALTY) / MAXTIME;

    IFlashLoan constant flashloanContract = IFlashLoan(0x...); // the address of the flashloan provider

    function attack(uint amount) external {
        uint penalty = (amount * PENALTY_RATE) / (10 ** 18);
        token.transferFrom(msg.sender, penalty); // assuming no flashloan fee
        IFlashLoan.flashloan(token, amount);
    }

    function flashloanCallback(uint amount) {
        votingEscrow.createLock(amount, block.timestamp + WEEK); // create a lock for a week with a very large of token

        // do whatever you want with your large amount of votes
        
        votingEscrow.quitLock();
        token.transfer(msg.sender, amount); // pay back the flashloan
    }
}

• The function names, arguments and return values might not be accurate and might change depends on the used flashloan platform, but this contract is just to give a general idea of an attack vector.

Tools Used

Manual auditing - VS Code and me :)

Recommended Mitigation Steps

Think about implementing a mechanism that prevents users from creating a lock and quitting it in the same transaction, that way attackers won't be able to use flashloan in order to achieve large voting power. However, regular loans will still be a problem with that fix, and if this isn't a wanted behavior addition fix is needed to be thought of.

QA Report

• Use accept ownership to avoid transferring ownership to a non-existing address. In the current implementation, if the transferOwnership function is called with an invalid address, the ownership will be transferred to this address and it can DoS the system. It will be better to make the new owner call a function before actually transferring the ownership, in order to validate that the new owner's address is valid and reachable.

• Lines 257-259 are redundant - lines 257-259 checks a condition and assigns a value to userPointHistory[_addr][uEpoch + 1] if the condition is true. However, a few lines after this assignment (in line 264), another value is assigned to userPointHistory[_addr][uEpoch + 1], which simply override the previous value. The previous value is not used between these two assignment, so the first assignment is redundant.

• Typo on line 270 - "by" is written instead of "be"

• Unspecific compiler version pragma - pragmas should be locked to a specific compiler version, to avoid contracts getting deployed using a different version, which may have a greater risk of undiscovered bugs.

Gas Optimizations

• Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc. An even better and gas efficient approach will be to use Solidity Custom Errors instead of revert strings.

• Add 1 to uEpoch in line 256 when initializing it instead of calculating uEpoch + 1 multiple times

• Cache decimals in the constructor instead of accessing the storage twice Current code:

  constructor(
        address _owner,
        address _penaltyRecipient,
        address _token,
        string memory _name,
        string memory _symbol
    ) {
        // ...
        
        decimals = IERC20(_token).decimals();
        require(decimals <= 18, "Exceeds max decimals");
  
        // ...
    }

  Optimized implementation:

    constructor(
        address _owner,
        address _penaltyRecipient,
        address _token,
        string memory _name,
        string memory _symbol
    ) {
        // ...
  
        uint _decimals = IERC20(_token).decimals();
        require(_decimals <= 18, "Exceeds max decimals");
        decimals = _decimals;
  
        // ...
    }

• Cache penaltyRecipient in collectPenalty to avoid accessing the storage twice Current version:

  function collectPenalty() external {
      uint256 amount = penaltyAccumulated;
      penaltyAccumulated = 0;
      require(token.transfer(penaltyRecipient, amount), "Transfer failed");
      emit CollectPenalty(amount, penaltyRecipient);
  }

  Optimized version:

  function collectPenalty() external {
      uint256 amount = penaltyAccumulated;
      penaltyAccumulated = 0;
      address _penaltyRecipient = penaltyRecipient;
      require(token.transfer(_penaltyRecipient, amount), "Transfer failed");
      emit CollectPenalty(amount, _penaltyRecipient);
  }

• Reduce the size of end in the LockedBalance struct so the struct will fit in 2 words instead of 3 (end is a timestamp and if we'll reach the maximum value of uint96 the contract probably won't be active due to the limited size of the Point arrays) Current struct:

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

  Optimized struct:

  // -------------- first word -----------------  --------------- second word ------------
  // [ amount - 128 bits | delegated - 128 bits]  [ delegatee - 160 bits | end - 96 bits ]
  struct LockedBalance {
        // amount and delegated are zipped to one word
        int128 amount;
        int128 delegated;
        // delegatee and end are zipped to the second word
        address delegatee;
        uint96 end;
    }

• No need to copy the locked balance in _delegate (all of its calls are in the end of the calling function, so the copied value won't actually be used after the function call)

• Move line 506 before line 503 in order to avoid accessing locked_.end twice

• Use Shift right/left instead of division/multiplication - in lines 720 and 744 you can optimize the calculation uint256 mid = (min + max + 1) / 2; by using shifting. The optimized version will look like this uint256 mid = (min + max + 1) >> 1; (right shifting by i is like dividing by 2**i, and left shifting by i is like multiplying by 2**i).

• Use != 0 instead of > 0 for unsigned integer comparison - this can be seen in multiple places in the code:

  • contracts/VotingEscrow.sol::288 => if (epoch > 0) {
  • contracts/VotingEscrow.sol::412 => require(_value > 0, "Only non zero amount");
  • contracts/VotingEscrow.sol::448 => require(_value > 0, "Only non zero amount");
  • contracts/features/Blocklist.sol::42 => return size > 0;

• Loops can be optimized in several ways. Let's take for example the loop in the _checkpoint function.

  for (uint256 i = 0; i < 255; i++) {
      // Hopefully it won't happen that this won't get used in 5 years!
      // If it does, users will be able to withdraw but vote weight will be broken
      iterativeTime = iterativeTime + WEEK;
      int128 dSlope = 0;
      if (iterativeTime > block.timestamp) {
          iterativeTime = block.timestamp;
      } else {
          dSlope = slopeChanges[iterativeTime];
      }
      int128 biasDelta =
          lastPoint.slope *
              int128(int256((iterativeTime - lastCheckpoint)));
      lastPoint.bias = lastPoint.bias - biasDelta;
      lastPoint.slope = lastPoint.slope + dSlope;
      // This can happen
      if (lastPoint.bias < 0) {
          lastPoint.bias = 0;
      }
      // This cannot happen - just in case
      if (lastPoint.slope < 0) {
          lastPoint.slope = 0;
      }
      lastCheckpoint = iterativeTime;
      lastPoint.ts = iterativeTime;
      lastPoint.blk =
          initialLastPoint.blk +
          (blockSlope * (iterativeTime - initialLastPoint.ts)) /
          MULTIPLIER;
  
      // when epoch is incremented, we either push here or after slopes updated below
      epoch = epoch + 1;
      if (iterativeTime == block.timestamp) {
          lastPoint.blk = block.number;
          break;
      } else {
          pointHistory[epoch] = lastPoint;
      }
  }

  We can do multiple things here:

  1. Variables in solidity are already initialized to their default value, and initializing them to the same value actually costs more gas. So for example in the loop above, the code can be optimized using uint256 i; instead of uint256 i = 0;. There is another redundant initialization of dSlope to 0.
  2. Use ++i and ++epoch instead of i++ and epoch++ to save some gas spent in every iteration.
  3. Use unchecked on the loop index incrementation. This is guaranteed to not overflow because it only goes from 0 to 255.
  4. Consider using iterativeTime as the loop index (and limited it by its initial value + 256 weeks) So after all these changes, the code will look something like this:

  for (uint256 i; i < 255; ) {
      // Hopefully it won't happen that this won't get used in 5 years!
      // If it does, users will be able to withdraw but vote weight will be broken
      iterativeTime = iterativeTime + WEEK;
      int128 dSlope;
      if (iterativeTime > block.timestamp) {
          iterativeTime = block.timestamp;
      } else {
          dSlope = slopeChanges[iterativeTime];
      }
      int128 biasDelta =
          lastPoint.slope *
              int128(int256((iterativeTime - lastCheckpoint)));
      lastPoint.bias = lastPoint.bias - biasDelta;
      lastPoint.slope = lastPoint.slope + dSlope;
      // This can happen
      if (lastPoint.bias < 0) {
          lastPoint.bias = 0;
      }
      // This cannot happen - just in case
      if (lastPoint.slope < 0) {
          lastPoint.slope = 0;
      }
      lastCheckpoint = iterativeTime;
      lastPoint.ts = iterativeTime;
      lastPoint.blk =
          initialLastPoint.blk +
          (blockSlope * (iterativeTime - initialLastPoint.ts)) /
          MULTIPLIER;
  
      // when epoch is incremented, we either push here or after slopes updated below
      ++epoch;
      if (iterativeTime == block.timestamp) {
          lastPoint.blk = block.number;
          break;
      } else {
          pointHistory[epoch] = lastPoint;
      }
      unchecked {
          ++i;
      }
  }

  If you'll choose to use `` as the loop index, it will look like this:

  uint timeLimit = iterativeTime + WEEK >> 8; // iterativeTime + WEEK * 256
  for (; iterativeTime < timeLimit; ) {
      // Hopefully it won't happen that this won't get used in 5 years!
      // If it does, users will be able to withdraw but vote weight will be broken
      unchecked {
          iterativeTime = iterativeTime + WEEK; // if it will overflow, the calculation of timeLimit would have overflow too
      }
      int128 dSlope;
      if (iterativeTime > block.timestamp) {
          iterativeTime = block.timestamp;
      } else {
          dSlope = slopeChanges[iterativeTime];
      }
      int128 biasDelta =
          lastPoint.slope *
              int128(int256((iterativeTime - lastCheckpoint)));
      lastPoint.bias = lastPoint.bias - biasDelta;
      lastPoint.slope = lastPoint.slope + dSlope;
      // This can happen
      if (lastPoint.bias < 0) {
          lastPoint.bias = 0;
      }
      // This cannot happen - just in case
      if (lastPoint.slope < 0) {
          lastPoint.slope = 0;
      }
      lastCheckpoint = iterativeTime;
      lastPoint.ts = iterativeTime;
      lastPoint.blk =
          initialLastPoint.blk +
          (blockSlope * (iterativeTime - initialLastPoint.ts)) /
          MULTIPLIER;
  
      // when epoch is incremented, we either push here or after slopes updated below
      ++epoch;
      if (iterativeTime == block.timestamp) {
          lastPoint.blk = block.number;
          break;
      } else {
          pointHistory[epoch] = lastPoint;
      }
  }

  There are more loops in the contract that can be optimized using the same optimizations.