FIAT DAO veFDT contest - cRat1st0s's results

Lines of code

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

Vulnerability details

2022-08-fiatdao-code4rena Report

• 2022-08-fiatdao-code4rena Report
  • Files Description Table
  • High Report
  • Issues found
    • [H-01]: Unsafe casts
      • Impact
      • Code Affected
      • Proof of Concept
      • Mitigation
      • Tools used

Files Description Table

High Report

Issues found

[H-01]: Unsafe casts

Impact

In functions createLock and increaseAmount there are unsafe casts with the parameter _value that is chosen by the caller. This can have unexpected consequences in delegator's voting power and/or delegatee's voting power.

Code Affected

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

locked_.amount += int128(int256(_value));

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

locked_.delegated += int128(int256(_value));

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

newLocked.amount += int128(int256(_value));

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

newLocked.delegated += int128(int256(_value));

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

locked_.amount += int128(int256(_value));

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

newLocked.delegated += int128(int256(_value));

Proof of Concept

 »  int128(int256(type(uint256).max))
-1
 »  int128(int256(type(uint256).max/1000000000))
-78873554377478089559772797583181133079

Mitigation

SafeCast library must be used in every type cast.

Tools used

VS Code

2022-08-fiatdao-code4rena Report

• 2022-08-fiatdao-code4rena Report
  • Files Description Table
  • QA Report
  • Issues found
  • [N-01]: Inconsistency between the code and the veFDT checkpoint math
    • Impact
    • Code Affected
    • Mitigation
    • Tools used
  • [N-02]: Inconsistency between the code and the veFDT checkpoint math
    • Impact
    • Code Affected
    • Mitigation
    • Tools used
  • [N-03]: Inconsistency between the code and the veFDT checkpoint math
    • Impact
    • Code Affected
    • Mitigation
    • Tools used

Files Description Table

QA Report

Issues found

[N-01]: Inconsistency between the code and the veFDT checkpoint math

Impact

Based on veFDT checkpoint math under require column is T>owner.end but in code is T>=owner.end.

Code Affected

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

require(unlock_time >= locked_.end, "Only increase lock end"); // from using quitLock, user should increaseAmount instead

Mitigation

Based on what is correct, do the appropriate changes.

Tools used

VS Code

[N-02]: Inconsistency between the code and the veFDT checkpoint math

Impact

Based on veFDT checkpoint math under update column is owner.amount=V but in code is owner.amount+=V.

Code Affected

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

locked_.amount += int128(int256(_value));

Mitigation

Based on what is correct, do the appropriate changes.

Tools used

VS Code

[N-03]: Inconsistency between the code and the veFDT checkpoint math

Impact

Based on veFDT checkpoint math under require column is to.end > from.end but in code is to.end >= from.end.

Code Affected

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

require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");

Mitigation

Based on what is correct, do the appropriate changes.

Tools used

VS Code

2022-08-fiatdao-code4rena Report

• 2022-08-fiatdao-code4rena Report
  • Files Description Table
  • Gas Optimizations
    • [G-01]: Variables: No need to explicitly initialize variables with default values
      • Impact
      • Code Affected:
      • Mitigation
      • Tools used
    • [G-02]: For-Loops: Pre-increments cost less gas compared to post-increments
      • Impact
      • Code Affected:
      • Mitigation
      • Tools used
    • [G-03]: For-Loops: Increments can be unchecked
      • Impact
      • Code Affected:
      • Mitigation
      • Tools used
    • [G-04]: For-Loops: No need to explicitly initialize variables with default values
      • Impact
      • Code Affected:
      • Mitigation
      • Tools used
    • [G-05]: Comparisons: Use != 0 rather than > 0 for unsigned integers in require() statements
      • Impact
      • Affected Code:
      • Mitigation
      • Tools used
    • [G-06]: Use Custom Errors
      • Impact
      • Mitigation
      • Tools used

Files Description Table

Gas Optimizations

[G-01]: Variables: No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0, etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas.

Code Affected:

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

int128 oldSlopeDelta = 0;

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

int128 newSlopeDelta = 0;

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

uint256 blockSlope = 0; // dblock/dt

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

int128 dSlope = 0;

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

uint256 min = 0;

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

uint256 min = 0;

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

uint256 dBlock = 0;

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

uint256 dTime = 0;

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

int128 dSlope = 0;

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

uint256 dTime = 0;

Mitigation

Do not initialize variables with default values.

Tools used

VS Code

[G-02]: For-Loops: Pre-increments cost less gas compared to post-increments

Impact

Pre-increments cost less gas compared to post-increments.

Code Affected:

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

for (uint256 i = 0; i < 255; i++) {

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

for (uint256 i = 0; i < 128; i++) {

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

for (uint256 i = 0; i < 128; i++) {

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

for (uint256 i = 0; i < 255; i++) {

Mitigation

Change i++ to ++i.

Tools used

VS Code

[G-03]: For-Loops: Increments can be unchecked

Impact

In Solidity 0.8+, there’s a default overflow check on unsigned integers.

Code Affected:

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

for (uint256 i = 0; i < 255; i++) {

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

for (uint256 i = 0; i < 128; i++) {

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

for (uint256 i = 0; i < 128; i++) {

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

for (uint256 i = 0; i < 255; i++) {

Mitigation

One example is the code would go from:

        for (uint256 i = 0; i < 128; i++) {
            if (min >= max) break;
            uint256 mid = (min + max + 1) / 2;
            if (pointHistory[mid].blk <= _block) {
                min = mid;
            } else {
                max = mid - 1;
            }
        }

to:

        for (uint256 i = 0; i < 128; ) {
            if (min >= max) break;
            uint256 mid = (min + max + 1) / 2;
            if (pointHistory[mid].blk <= _block) {
                min = mid;
            } else {
                max = mid - 1;
            }
            unchecked {
                i++;
            }
        }

Tools used

VS Code

[G-04]: For-Loops: No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0, etc depending on the data type). If you explicitly initialize it with its default value, you are just wasting gas.

Code Affected:

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

for (uint256 i = 0; i < 255; i++) {

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

for (uint256 i = 0; i < 128; i++) {

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

for (uint256 i = 0; i < 128; i++) {

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

for (uint256 i = 0; i < 255; i++) {

Mitigation

Do not initialize variables with default values.

Tools used

VS Code

[G-05]: Comparisons: Use != 0 rather than > 0 for unsigned integers in require() statements

Impact

When the optimizer is enabled, gas is wasted by doing a greater-than operation, rather than a not-equals operation inside require() statements. When using !=, the optimizer is able to avoid the EQ, ISZERO, and associated operations, by relying on the JUMPI that comes afterwards, which itself checks for zero.

Affected Code:

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

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

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

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

Mitigation

Use != 0 rather than > 0 for unsigned integers in require() statements.

Tools used

VS Code

[G-06]: Use Custom Errors

Impact

Less expensive and able to use dynamic information in them.

Mitigation

Use custom errors.

Tools used

VS Code