Back to reassor's contests

FIAT DAO veFDT contest - reassor's results

Unlock liquidity for your DeFi fixed income assets.

General Information

Platform: Code4rena

Start Date: 12/08/2022

Pot Size: $35,000 USDC

Total HM: 10

Participants: 126

Period: 3 days

Judge: Justin Goro

Total Solo HM: 3

Id: 154

League: ETH

FIAT DAO

Findings Distribution

Researcher Performance

Rank: 7/126

Findings: 4

Award: $577.06

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryFIAT DAO

Findings Information

🌟 Selected for report: JohnSmith

Also found by: ayeslick, reassor, rokinot, scaraven

Labels

bug
duplicate
2 (Med Risk)

Awards

389.9867 USDC - $389.99

External Links

Link to GitHub issue

Lines of code

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

Vulnerability details

Impact

Contract Blocklist implements functionality for adding contract address to the blocklist and any address that is on the blocklist is blocked from using some of VotingEscrow contract functionality.

The issue is that in order to add address to blocklist, the address is checked if its a contract address by checking its codesize. This check can be bypassed using selfdestruct/create2 attack.

function _isContract(address addr) internal view returns (bool) {
    uint256 size;
    assembly {
        size := extcodesize(addr)
    }
    return size > 0;
}

Attack Scenario:

  1. Attacker creates Exploit contract that can be destroyed and deployed to the same address using create2 opcode.
  2. Owner of Blocklist decides to add Exploit address to blocklist through block function.
  3. Attacker monitors mempool and performs front-running attack to destroy Exploit contract before owner's block transaction.
  4. Owner's block transaction fails
  5. Attacker redeploys Exploit contract whenever he needs it functionality

Attacker also can skip part of front-running attack and just destroy Exploit contract after interacting with VotingEscrow contract and only redeploy it when he needs it. This will result in owner not being able to block Exploit contract in any way.

Proof of Concept

features/Blocklist.sol

Tools Used

Manual Review / VSCode

It is recommended to consider removing check in Blocklist.block function for contract address and allow owner to also block EOA accounts.

#0 - lacoop6tu

2022-08-16T12:34:15Z

Duplicate of #168

#1 - gititGoro

2022-08-31T02:09:40Z

Duplicate of #75. Risk downgraded.

Findings Information

🌟 Selected for report: CertoraInc

Also found by: 0x1f8b, DecorativePineapple, cRat1st0s, carlitox477, joestakey, ladboy233, reassor, rvierdiiev

Labels

bug
duplicate
2 (Med Risk)

Awards

142.1501 USDC - $142.15

External Links

Link to GitHub issue

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#L426 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L460-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 https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L486

Vulnerability details

Impact

Contract VotingEscrow implements functions createLock and increaseAmount that follow similar logic:

  1. It is required to pass _value parameter
  2. locked._amount is set/increased int128(int256(_value))
  3. locked._delegated is set/increased int128(int256(_value))
  4. Transfer of tokens is triggered: token.transferFrom(msg.sender, address(this), _value)

The issue is that if the uint256 _value is bigger than int128 then it will be assigned to user value that fits in int128 but the user will loose the _value of tokens that exceeds int128.

Proof of Concept

VotingEscrow.sol:

Tools Used

Manual Review / VSCode

It is recommended to perform casting of _value to int128 and then use its result value for locked._amount, locked._delegated and also for transfer of tokens itself (by casting it back to uint256 value).

#0 - bahurum

2022-08-16T22:17:05Z

Duplicate of #228

#1 - lacoop6tu

2022-08-17T10:29:49Z

Duplicate of #228

#2 - gititGoro

2022-09-02T00:13:26Z

Duplicate upheld

Findings Information

🌟 Selected for report: oyc_109

Also found by: 0x1f8b, 0x52, 0xDjango, 0xLovesleep, 0xNazgul, 0xNineDec, 0xbepresent, 0xmatt, 0xsolstars, Aymen0909, Bahurum, Bnke0x0, CertoraInc, Chom, CodingNameKiki, DecorativePineapple, Deivitto, Dravee, ElKu, Funen, GalloDaSballo, IllIllI, JC, JohnSmith, Junnon, KIntern_NA, Lambda, LeoS, MiloTruck, Noah3o6, PaludoX0, RedOneN, Respx, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, Ruhum, Sm4rty, TomJ, Vexjon, Waze, Yiko, __141345__, a12jmx, ajtra, ak1, apostle0x01, asutorufos, auditor0517, bin2chen, bobirichman, brgltd, bulej93, byndooa, c3phas, cRat1st0s, cryptphi, csanuragjain, d3e4, defsec, delfin454000, djxploit, durianSausage, ellahi, erictee, exd0tpy, fatherOfBlocks, gogo, jonatascm, ladboy233, medikko, mics, natzuu, neumo, p_crypt0, paribus, pfapostol, rbserver, reassor, ret2basic, robee, rokinot, rvierdiiev, sach1r0, saneryee, seyni, sikorico, simon135, sseefried, wagmi, wastewa

Awards

29.9451 USDC - $29.95

Labels

bug
QA (Quality Assurance)

External Links

Link to GitHub issue

List

Low Risk

  1. Missing events
  2. Missing zero address checks
  3. Critical address change

Non-Critical Risk

  1. Missing indexed events
  2. Missing pause functionality
  3. Contracts are using unlocked pragma
  4. Missing/incomplete natspec comments
  5. Use scientific notation for big numbers

1. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

features/Blocklist.sol:

VotingEscrow.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing events to listed functions.

2. Missing zero address checks

Risk

Low

Impact

Multiple contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

features/Blocklist.sol:

VotingEscrow.sol:

Tools Used

Manual Review / VSCode

It is recommended to add zero address checks for listed parameters.

3. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

VotingEscrow.sol:

Tools Used

Manual Review / VSCode

It is recommended to implement two-step process for changing ownership.

4. Missing indexed events

Risk

Non-Critical

Impact

Contract VotingEscrow implements multiple events but does not properly index parameters. Lack of indexed parameters for events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

VotingEscrow.sol:38:    event TransferOwnership(address owner);
VotingEscrow.sol:39:    event UpdateBlocklist(address blocklist);
VotingEscrow.sol:40:    event UpdatePenaltyRecipient(address recipient);
VotingEscrow.sol:41:    event CollectPenalty(uint256 amount, address recipient);

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add indexed keyword to listed events parameters.

5. Missing pause functionality

Risk

Non-Critical

Impact

Contracts are missing pause functionality that could be used in case of security incidents and would block executing selected functions while the contract is paused.

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to add functionality for pausing contracts and go through all publicly/externally accessible functions to decide which one should be forbidden from running while the contracts are paused.

6. Contracts are using unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

VotingEscrow.sol:2:pragma solidity ^0.8.3;
features/Blocklist.sol:2:pragma solidity ^0.8.3;
interfaces/IERC20.sol:2:pragma solidity ^0.8.3;
interfaces/IVotingEscrow.sol:2:pragma solidity ^0.8.3;
interfaces/IBlocklist.sol:2:pragma solidity ^0.8.3;

Tools Used

Manual Review / VSCode

Consider locking compiler version, for example pragma solidity 0.8.4.

7. Missing/incomplete natspec comments

Risk

Non-Critical

Impact

Multiple contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

features/Blocklist.sol:

VotingEscrow.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing natspec comments.

8. Use scientific notation for big numbers

Risk

Non-Critical

Impact

Contract VotingEscrow is using big numbers which is prone to mistakes:

Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
mapping(address => Point[1000000000]) public userPointHistory;

Proof of Concept

VotingEscrow.sol:

Tools Used

Manual Review / VSCode

It is recommended to use scientific notation, for example: 1e18.

Point[1e18] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
mapping(address => Point[1e9]) public userPointHistory;

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x040, 0x1f8b, 0xDjango, 0xHarry, 0xLovesleep, 0xNazgul, 0xNineDec, 0xSmartContract, 0xackermann, 0xbepresent, 2997ms, Amithuddar, Aymen0909, Bnke0x0, CRYP70, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, Fitraldys, Funen, GalloDaSballo, JC, JohnSmith, Junnon, LeoS, Metatron, MiloTruck, Noah3o6, NoamYakov, PaludoX0, RedOneN, Respx, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, SooYa, SpaceCake, TomJ, Tomio, Waze, Yiko, __141345__, a12jmx, ajtra, ak1, apostle0x01, asutorufos, bobirichman, brgltd, bulej93, c3phas, cRat1st0s, carlitox477, chrisdior4, csanuragjain, d3e4, defsec, delfin454000, djxploit, durianSausage, ellahi, erictee, fatherOfBlocks, gerdusx, gogo, ignacio, jag, ladboy233, m_Rassska, medikko, mics, natzuu, newfork01, oyc_109, paribus, pfapostol, rbserver, reassor, ret2basic, robee, rokinot, rvierdiiev, sach1r0, saian, sashik_eth, sikorico, simon135

Awards

14.9678 USDC - $14.97

Labels

bug
G (Gas Optimization)

External Links

Link to GitHub issue

List

  1. Pack integer values and reorder struct variables
  2. Obsolete require check
  3. Use scientific notation
  4. Use custom errors instead of revert strings to save gas
  5. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1
  6. Obsolete overflow/underflow check
  7. No need to explicitly initialize variables with default values
  8. Use != 0 instead of > 0 for unsigned integer comparison
  9. Use Shift Right/Left instead of Division/Multiplication if possible

1. Pack integer values and reorder struct variables

Impact

Packing variables into storage slots saves gas. Variable LockedBalance.end that is expected to hold block timestamp can be easily packed in uint96 type and reordering LockedBalance struct will result in using only two storage slots.

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

Proof of Concept

VotingEscrow.sol:

Tools Used

Manual Review / VSCode

It is recommended to pack listed integer variables and reorder struct variables.

2. Obsolete require check

Impact

Function VotingEscrow.createLocker implements require check to make sure that unlock_time is bigger or equal to locked_.end. This check can be removed since locked_.end always will hold value 0:

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

There is already check if unlock_time is bigger than block.timestamp.

Proof of Concept

VotingEscrow.sol:

It is recommended to remove require check:

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

3. Use scientific notation

Impact

Contract VotingEscrow is using math exponent calculation to express big numbers. This consumes additional gas and it is better to use scientific notation.

Proof of Concept

VotingEscrow.sol:48:    uint256 public constant MULTIPLIER = 10**18;
VotingEscrow.sol:51:    uint256 public maxPenalty = 10**18; // penalty for quitters with MAXTIME remaining lock
VotingEscrow.sol:653:        uint256 penaltyAmount = (value * penaltyRate) / 10**18; // quitlock_penalty is in 18 decimals precision
VotingEscrow.sol:57:    Point[1000000000000000000] public pointHistory; // 1e9 * userPointHistory-length, so sufficient for 1e9 users
VotingEscrow.sol:58:    mapping(address => Point[1000000000]) public userPointHistory;

It is recommended to use scientific notation, for example: 1e18.

4. Use custom errors instead of revert strings to save gas

Impact

Usage of custom errors reduces the gas cost.

Proof of Concept

Contracts that should be using custom errors:

  • VotingEscrow.sol
  • Blocklist.sol

It is recommended to add custom errors to listed contracts.

5. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

VotingEscrow.sol:309:        for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol:717:        for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol:739:        for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol:834:        for (uint256 i = 0; i < 255; i++) {

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

6. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

VotingEscrow.sol:309:        for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol:717:        for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol:739:        for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol:834:        for (uint256 i = 0; i < 255; i++) {

It is recommended to wrap incrementing with unchecked block, for example: unchecked { ++i } or unchecked { --i }

7. 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 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

VotingEscrow.sol:298:        uint256 blockSlope = 0; // dblock/dt
VotingEscrow.sol:309:        for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol:714:        uint256 min = 0;
VotingEscrow.sol:717:        for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol:737:        uint256 min = 0;
VotingEscrow.sol:739:        for (uint256 i = 0; i < 128; i++) {
VotingEscrow.sol:793:        uint256 dBlock = 0;
VotingEscrow.sol:794:        uint256 dTime = 0;
VotingEscrow.sol:834:        for (uint256 i = 0; i < 255; i++) {
VotingEscrow.sol:889:        uint256 dTime = 0;

It is recommended to remove explicit initializations with default values.

8. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

VotingEscrow.sol:176:        if (delegatee != _addr && value > 0) {
VotingEscrow.sol:236:            if (_oldLocked.end > block.timestamp && _oldLocked.delegated > 0) {
VotingEscrow.sol:244:            if (_newLocked.end > block.timestamp && _newLocked.delegated > 0) {
VotingEscrow.sol:288:        if (epoch > 0) {
VotingEscrow.sol:412:        require(_value > 0, "Only non zero amount");
VotingEscrow.sol:448:        require(_value > 0, "Only non zero amount");
VotingEscrow.sol:449:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:469:            require(locked_.amount > 0, "Delegatee has no lock");
VotingEscrow.sol:502:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:529:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:564:        require(locked_.amount > 0, "No lock");
VotingEscrow.sol:587:        require(toLocked.amount > 0, "Delegatee has no lock");
VotingEscrow.sol:621:        if (newLocked.amount > 0) {
VotingEscrow.sol:635:        require(locked_.amount > 0, "No lock");
features/Blocklist.sol:42:        return size > 0;

It is recommended to use != 0 instead of > 0.

9. Use Shift Right/Left instead of Division/Multiplication if possible

Impact

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

Bad:

uint256 b = a / 2;

Good:

uint256 b = a >> 1;

Proof of Concept

VotingEscrow.sol:

It is recommended to use shift right/left.

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter