Inverse Finance contest - JC's results

Rethink the way you borrow.

General Information

Platform: Code4rena

Start Date: 25/10/2022

Pot Size: $50,000 USDC

Total HM: 18

Participants: 127

Period: 5 days

Judge: 0xean

Total Solo HM: 9

Id: 175

League: ETH

Inverse Finance

Findings Distribution

Researcher Performance

Rank: 67/127

Findings: 2

Award: $55.74

QA:

grade-b

Gas:

grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Inverse Finance

Findings Information

🌟 Selected for report: 0x1f8b

Also found by: 0xNazgul, 0xSmartContract, Aymen0909, B2, Bnke0x0, Deivitto, Diana, Dinesh11G, ElKu, JC, Josiah, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Waze, __141345__, adriro, aphak5010, brgltd, c3phas, c7e7eff, carlitox477, cducrest, ch0bu, chrisdior4, cryptonue, cryptostellar5, cylzxje, d3e4, delfin454000, enckrish, evmwanderer, fatherOfBlocks, gogo, hansfriese, horsefacts, immeas, leosathya, lukris02, neumo, oyc_109, pedr02b2, rbserver, robee, rotcivegaf, rvierdiiev, sakshamguruji, shark, simon135, tnevler, trustindistrust, wagmi

Awards

36.7345 USDC - $36.73

Labels

bug

QA (Quality Assurance)

grade-b

Q-54

External Links

Link to GitHub issue

summary

N-01 public functions not called by the contract should be declared external instead | 52 instances
N-02 Missing event for critical parameter change | 12 instances
N-03 Use a more recent version of solidity | 2 instances
N-04 NatSpec is incomplete | 2 instances
N-05 Event is missing indexed fields | 6 instances
N-06 Open TODO | 1 instance
N-07 The visibility for constructor is ignored | 6 instances
N-08 Use of ecrecover is susceptible to signature malleability | 1 instance
N-09 Large multiples of ten should use scientific notation rather than decimal literals for readability | 3 instances

Sub- Total: 84 instances in 8 issues

L-01 Missing checks for address(0x0) when assigning values to address state variables | 8 instances
L-02 abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 3 instances
L-03 FeeOnTransfer Tokens not Supported | 2 instances
L-04 Unspecific Compiler Version Pragma | 8 instances

Sub-total: 21 instances in 4 issues

Total: 105 instances in 12 issues

N-01 public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents’ functions and change the visibility from external to public. https://docs.soliditylang.org/en/latest/contracts.html#function-overriding

52 instances in 8 files:

BorrowController.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L26 https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L32 https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L38 https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L46

Oracle.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L44 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L53 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L66

GovTokenEscrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L30 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L43 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L52 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L66

INVEscrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L44 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L59 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L70 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L79 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L90

SimpleERC20Escrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L25 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L36 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L45

N-02 Missing event for critical parameter change

12 instances in 4 files:

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L6 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L17 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L19

Fed.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L11 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L12 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L14

GovTokenEscrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L6 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L7 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L9 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L10

SimpleERC20Escrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L6 https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L7

N-03 Use a more recent version of solidity

Use a solidity version of at least 0.8.14 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)

2 instances:

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L2 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L2

N-04 NatSpec is incomplete

2 instances in 1 file:

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L96 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L100

N-05 Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

6 instances in 2 files:

DBR.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L381 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L382

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L616 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L617 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L618 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L619

N-06 Open TODO

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L35

N-07 The visibility for constructor is ignored

6 instances in 6 files:

DBR.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L30-L42

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L61-L90

Fed.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L36-L42

Oracle.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L29-L33

BorrowController.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L13-L15

INVEscrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L34-L36

N-08 Use of ecrecover is susceptible to signature malleability

The built-in EVM precompile ecrecover is susceptible to signature malleability, which could lead to replay attacks

References:

Recommend considering using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function.

1 instance in 1 file:

DBR.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L226

N-09 Large multiples of ten should use scientific notation rather than decimal literals for readability

3 instances in 1 file:

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L74 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L75 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L76

L-01 Missing checks for address(0x0) when assigning values to address state variables

8 instances in 4 files:

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L77 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L78 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L79 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L80

Fed.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L39 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L40

Oracle.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L32

BorrowController.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L14

L-02 abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). “Unless there is a compelling reason, abi.encode should be preferred”. If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

3 instances in 2 files:

DBR.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L228

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L427 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L491

L-03 FeeOnTransfer Tokens not Supported

There exist ERC20 tokens that charge a fee for every transfer() or transferFrom(). If this tokens are unsupported, ensure there is proper documentation about it...

2 instances in 2 file:

INVEscrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L63

SimpleERC20Escrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L38

L-04 Unspecific Compiler Version Pragma

Avoid floating pragmas for non-library contracts. While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain. It is recommended to pin to a concrete compiler version, e.g. 'pragma solidity ^0.8.0;' -> 'pragma solidity 0.8.4;"

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L2

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L2

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L2

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L2

https://github.com/code-423n4/2022-10-inverse/blob/main/src/BorrowController.sol#L2

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/GovTokenEscrow.sol#L2

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L2

https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L2

#0 - c4-judge
2022-11-08T00:45:17Z

0xean marked the issue as grade-b

Findings Information

🌟 Selected for report: pfapostol

Also found by: 0x1f8b, 0xRoxas, 0xSmartContract, Amithuddar, Aymen0909, B2, Bnke0x0, Chandr, CloudX, Deivitto, Diana, Dinesh11G, ElKu, HardlyCodeMan, JC, JrNet, KoKo, Mathieu, Ozy42, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Shinchan, __141345__, adriro, ajtra, aphak5010, ballx, c3phas, carlitox477, ch0bu, chaduke, cryptostellar5, djxploit, durianSausage, enckrish, exolorkistis, fatherOfBlocks, gogo, horsefacts, kaden, karanctf, leosathya, martin, mcwildy, oyc_109, ret2basic, robee, sakman, sakshamguruji, shark, skyle, tnevler

Awards

19.0072 USDC - $19.01

Labels

bug

G (Gas Optimization)

grade-b

G-53

External Links

Link to GitHub issue

Summary

G-01 Multiple address mappings can be combined into a single mapping of an address to a struct | 14 instances
G-02 State variables only set in the constructor should be declared immutable | 2 instances
G-03 State variables can be packed into fewer storage slots | 1 instances
G-04 <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 15 instances
G-05 Splitting require() statements that use && saves gas | 7 instances
G-06 Public functions to external | 52 instances
G-07 Use simple comparison in trinary logic / in if statement | 15 instances
G-08 Amounts should be checked for 0 before calling a transfer | 6 instances

Total: 112 instances in 8 issues

G-01 Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

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.

14 instances in 3 files:

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L57 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L58 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L59

Oracle.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L25 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L26 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L27

G-02 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)

2 instances in 1 file:

DBR.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L11 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L12

G-03 State variables can be packed into fewer storage slots

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract’s storage and therefore reduce deployment costs. address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous). As bool type variables are of size 1 byte, there’s a slot here that can get saved by moving them closer to an address

2 instances in 2 files:

DBR.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L24-L25

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L52-L53

G-04 <x> += <y> costs more gas than <x> = <x> + <y> for state variables

15 instances in 3 files:

Fed.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L91 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L92

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L395 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L397 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L565 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L568 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L598

G-05 Splitting require() statements that use && saves gas

Instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement (saving 3 gas per &).

7 instances in 1 file:

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L75 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L162 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L173 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L184 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L195 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L448 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L512

G-06 Public functions to external

Public functions not called by the contract should be declared external instead. Contracts are allowed to override their parents’ functions and change the visibility from external to public and can save gas by doing so. https://docs.soliditylang.org/en/latest/contracts.html#function-overriding

52 instances in 8 files:

G-07 Use simple comparison in trinary logic / in if statement

The comparison operators >= and <= use more gas than >, <, or ==. Replacing the >= and ≤ operators with a comparison operator that has an opcode in the EVM saves gas.

Recommended Mitigation Steps: Replace the comparison operator and reverse the logic to save gas using the suggestions above.

15 instances in 3 files:

DBR.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L171 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L195 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L224 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L329 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L373

Fed.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L123

G-08 Amounts should be checked for 0 before calling a transfer

Checking non-zero transfer values can avoid an expensive external call and save gas. While this is done at some places, it’s not consistently done in the solution.

Recommended Mitigation Steps: I suggest adding a non-zero-value before calling a transfer.

6 instances in 2 files:

Market.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L205 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L399 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L570

Fed.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/Fed.sol#L135

INVEscrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L63

SimpleERC20Escrow.sol https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/SimpleERC20Escrow.sol#L38

#0 - c4-judge
2022-11-05T23:54:43Z

0xean marked the issue as grade-b

Inverse Finance contest - JC's results

Rethink the way you borrow.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-54] QA Report - $36.73

Findings Information

Awards

Labels

External Links

summary

N-01 public functions not called by the contract should be declared external instead

N-02 Missing event for critical parameter change

N-03 Use a more recent version of solidity

N-04 NatSpec is incomplete

N-05 Event is missing indexed fields

N-06 Open TODO

N-07 The visibility for constructor is ignored

N-08 Use of ecrecover is susceptible to signature malleability

N-09 Large multiples of ten should use scientific notation rather than decimal literals for readability

L-01 Missing checks for address(0x0) when assigning values to address state variables

L-02 abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

L-03 FeeOnTransfer Tokens not Supported

L-04 Unspecific Compiler Version Pragma

#0 - c4-judge2022-11-08T00:45:17Z

[G-53] Gas Report - $19.01

Findings Information

Awards

Labels

External Links

Summary

G-01 Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

G-02 State variables only set in the constructor should be declared immutable

G-03 State variables can be packed into fewer storage slots

G-04 <x> += <y> costs more gas than <x> = <x> + <y> for state variables

G-05 Splitting require() statements that use && saves gas

G-06 Public functions to external

G-07 Use simple comparison in trinary logic / in if statement

G-08 Amounts should be checked for 0 before calling a transfer

#0 - c4-judge2022-11-05T23:54:43Z

#0 - c4-judge
2022-11-08T00:45:17Z

#0 - c4-judge
2022-11-05T23:54:43Z