FIAT DAO veFDT contest - Aymen0909's results

Lines of code

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

Vulnerability details

Impact

The potentiel impact of this error are :

• Give wrong voting power to a user at a given block.
• Give wrong total voting power at a given block.
• Give wrong total voting power.

Proof of Concept

The error occured in this line : https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L513

In the increaseUnlockTime function the oldLocked.end passed to the function _checkpoint is wrong as it is the same as the new newLock end time (called unlock_time) instead of being equal to oldUnlockTime .

In the given CheckpointMath.md file it is stated that checkpoint details for increaseUnlockTime function should be :

BUT with this error you get a different checkpoint details :

The error is illustrated in the code below :

        LockedBalance memory locked_ = locked[msg.sender];
        uint256 unlock_time = _floorToWeek(_unlockTime); // Locktime is rounded down to weeks
        /* @audit comment
                 the unlock_time represent the newLock end time
        */
        // Validate inputs
        require(locked_.amount > 0, "No lock");
        require(unlock_time > locked_.end, "Only increase lock end");
        require(unlock_time <= block.timestamp + MAXTIME, "Exceeds maxtime");
        // Update lock
        uint256 oldUnlockTime = locked_.end;
        locked_.end = unlock_time;
        /* @audit comment
                 The locked_ end time is update from  oldUnlockTime  ==>  unlock_time
        */
        locked[msg.sender] = locked_;
        if (locked_.delegatee == msg.sender) {
            // Undelegated lock
            require(oldUnlockTime > block.timestamp, "Lock expired");
            LockedBalance memory oldLocked = _copyLock(locked_);
            oldLocked.end = unlock_time;
            /* @audit comment
                 The oldLocked.end is set to unlock_time instead of   oldUnlockTime 
            */
            _checkpoint(msg.sender, oldLocked, locked_);
        }

The impact of this is when calculating the userOldPoint.bias in the _checkpoint function you get an incorrect value equal to userNewPoint.bias (because oldLocked.end == _newLocked.end which is wrong).

240        userOldPoint.bias =
241                    userOldPoint.slope *
242                    int128(int256(_oldLocked.end - block.timestamp));

The wrong userOldPoint.bias value is later used to calculate and update the bias value for the new point in PointHistory.

359       lastPoint.bias =
360                  lastPoint.bias +
361                  userNewPoint.bias -
362                  userOldPoint.bias;

372       pointHistory[epoch] = lastPoint;

And added to that the wrong oldLocked.end is used to get oldSlopeDelta value which is used to update the slopeChanges.

271       oldSlopeDelta = slopeChanges[_oldLocked.end];

380       oldSlopeDelta = oldSlopeDelta + userOldPoint.slope;
381       if (_newLocked.end == _oldLocked.end) {
382                  oldSlopeDelta = oldSlopeDelta - userNewPoint.slope; // It was a new deposit, not extension
383        }
384       slopeChanges[_oldLocked.end] = oldSlopeDelta;

As the PointHistory and the slopeChanges values are used inside the functions balanceOfAt() , _supplyAt(), totalSupply(), totalSupplyAt() to calculate the voting power, THIS ERROR COULD GIVE WRONG VOTING POWER AT A GIVEN BLOCK OF A USER OR CAN GIVE WRONG TOTAL VOTING POWER.

Tools Used

Manual Audit

Recommended Mitigation Steps

The line 513 in the VotingEscrow.sol contract :

      513      oldLocked.end = unlock_time;

Need to be replaced with the following :

      513      oldLocked.end = oldUnlockTime;

Low Risk & QA

Summary

Findings

1- Missing Zero address checks :

*Impact - LOW RISK

There is no ZERO address checks for transferOwnership function inside VoteEscrow.sol contract, so if zero address is passed by error the contract control will be lost.

The same issue appear in the constructor of the Blocklist.sol contract, there is no ZERO address check for the manager or ve contract addresses, if zero address is passed for the manager by error there no way anymore to block a contract from the VoteEscrow

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol

139      owner = _addr;

File: contracts/features/Blocklist.sol

139      manager = _manager;
140      ve = _ve;

2- Contract Blocking lack decentralization :

*Impact - LOW RISK

If the manager of Blocklist.sol contract is an EOA then he has all the power to block any contract and force it to undelegate from VoteEscrow, which is very centralized.

The manager should be another smart contract or a multisig contract where a group of people elected by DAO voting are responsible for voting if a contract must be blocked or not , and then the manager contract calls the block function.

3- Public functions not called by the contract should be declared external instead :

*Impact - NON CRITICAL

There is 1 instance of this issue:

File: contracts/features/Blocklist.sol

33     function isBlocked(address addr) public view returns (bool)

4- Large multiples of 10 should use scientific notation (eq 1e6) rather than decimal literals or exponentiation (e.g. 1000000, 10**18) :

*Impact - NON CRITICAL

When using multiples of 10 you shouldn't use decimal literals or exponentiation but use scientific notation for better readability.

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

46      uint256 public constant MULTIPLIER = 10**18;
49      uint256 public maxPenalty = 10**18;
55      Point[1000000000000000000] public pointHistory;
56      mapping(address => Point[1000000000]) public userPointHistory;

5- Duplicated require() checks should be refactored to a modifier for saving deployment costs :

*Impact - NON CRITICAL

require() checks repeated in multiple functions should be refactored into a modifier for better readability and also to save deployment gas.

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

140     require(msg.sender == owner, "Only owner");
147     require(msg.sender == owner, "Only owner");
154     require(msg.sender == owner, "Only owner");
162     require(msg.sender == owner, "Only owner");

Those checks should be replaced by a onlyOwner modifier.

6- Named return variables not used anywhere in the function :

*Impact - NON CRITICAL

Named return variable should be used inside the function or if not they should be removed to avoid confusion.

There is 1 instance of this issue in the getLastUserPoint function :

https://github.com/code-423n4/2022-08-fiatdao/blob/main/contracts/VotingEscrow.sol#L201-L216

7- Use more recent solidity versions :

*Impact - NON CRITICAL

Use a solidity version of at least 0.8.4 to get Custom errors which are used in place of require()/revert() strings to save deployment cost.

File: contracts/VotingEscrow.sol

2      pragma solidity ^0.8.3;

File: contracts/features/Blocklist.sol

2      pragma solidity ^0.8.3;

Gas Optimizations

Summary

Findings

1- Multiple address mappings can be combined into a single mapping of an address to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol

58      mapping(address => Point[1000000000]) public userPointHistory;
59      mapping(address => uint256) public userPointEpoch;
60      mapping(address => LockedBalance) public locked;

These mappings could be refactored into the following struct and mapping for example :

struct UserInfo {
        Point[1000000000] userPointHistory;
        uin256 userPointEpoch;
        LockedBalance locked;
    }
mapping(address => UserInfo) public usersInfo;

2- State variables only set in the constructor should be declared immutable :

Avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There are 5 instances of this issue:

File: contracts/VotingEscrow.sol

64      string public name;
65      string public symbol;
66      uint256 public decimals = 18;

File: contracts/features/Blocklist.sol

11      address public manager;
12      address public ve;

3- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, this saves gas when saving to storage

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

75      struct LockedBalance {
76          int128 amount;
77          uint256 end;
78          int128 delegated;
79          address delegatee;
80      }

It should be rearranged as follow :

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

This saves approx 20000 gas for creating a Lock and 10000 gas in deployment cost as the Gas test performed using the "votingEscrowGasTest.ts" file shows :

4- Avoid contract existence checks by using solidity version 0.8.10 or later :

Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (700 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value.

There are 2 instances of this issue:

File: contracts/VotingEscrow.sol

125     require(!IBlocklist(blocklis).isBlocked(msg.sender),"Blocked contract");
563     require(!IBlocklist(blocklist).isBlocked(_addr), "Blocked contract");

5- Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead :

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size as you can check here.

So use uint256/int256 for state variables and then downcast to lower sizes where needed.

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

60      mapping(uint256 => int128) public slopeChanges;
229     int128 oldSlopeDelta = 0;
230     int128 newSlopeDelta = 0;
313     int128 dSlope = 0;

6- Duplicated require() checks should be refactored to a modifier for saving deployment costs :

require() checks repeated in multiple functions should be refactored into a modifier to save deployment gas.

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

140     require(msg.sender == owner, "Only owner");
147     require(msg.sender == owner, "Only owner");
154     require(msg.sender == owner, "Only owner");
162     require(msg.sender == owner, "Only owner");

Those checks should be replaced by a onlyOwner modifier.

7- It costs more gas to initialize variables to zero than to let the default of zero be applied :

If a variable is not set/initialized, it is assumed to have the default value (0 for uint or int, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

There are 14 instances of this issue:

File: contracts/VotingEscrow.sol

229     int128 oldSlopeDelta = 0;
230     int128 newSlopeDelta = 0;
298     uint256 blockSlope = 0;
309     for (uint256 i = 0; i < 255; i++)
313     int128 dSlope = 0;
714     uint256 min = 0;
717     for (uint256 i = 0; i < 128; i++)
737     uint256 min = 0;
739     for (uint256 i = 0; i < 128; i++)
793     uint256 dBlock = 0;
794     uint256 dTime = 0;
834     for (uint256 i = 0; i < 255; i++)
836     int128 dSlope = 0
889     uint256 dTime = 0;

8- Use of ++i cost less gas than i++ in for loops :

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

309     for (uint256 i = 0; i < 255; i++)
717     for (uint256 i = 0; i < 128; i++)
739     for (uint256 i = 0; i < 128; i++)
834     for (uint256 i = 0; i < 255; i++)

9- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

309     for (uint256 i = 0; i < 255; i++)
717     for (uint256 i = 0; i < 128; i++)
739     for (uint256 i = 0; i < 128; i++)
834     for (uint256 i = 0; i < 255; i++)

10- Using > 0 costs more gas than != 0 when used on a uint in a require() statement :

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

412     require(_value > 0, "Only non zero amount");
448     require(_value > 0, "Only non zero amount");

11- X += Y costs more gas than X = X + Y for state variables :

There is 1 instance of this issue:

File: contracts/VotingEscrow.sol

654     penaltyAccumulated += penaltyAmount;

12- Using private rather than public for constants, saves gas :

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

There are 3 instances of this issue:

File: contracts/VotingEscrow.sol
46     uint256 public constant WEEK = 7 days;
47     uint256 public constant MAXTIME = 365 days;
48     uint256 public constant MULTIPLIER = 10**18;

13- Public functions not called by the contract should be declared external instead :

There is 1 instance of this issue:

File: contracts/features/Blocklist.sol

33     function isBlocked(address addr) public view returns (bool)

14- Functions guaranteed to revert when called by normal users can be marked payable :

If a function modifier such as onlyAdmin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2). Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 4 instances of this issue:

File: contracts/VotingEscrow.sol

139     function transferOwnership(address _addr) external
146     function updateBlocklist(address _addr) external
153     function updatePenaltyRecipient(address _addr) external
161     function unlock() external