Debt DAO contest - 0x1f8b's results

• Low
  • 1. Mixing and Outdated compiler
  • 2. Open TODO
  • 3. Lack of address checks
  • 4. Don't reduce safety for gas savings
  • 5. Send ether in an un-compatible way
• Non critical
  • 6. Add missing @param information
  • 7. Wrong return

Low

1. Mixing and Outdated compiler

The pragma version used are:

pragma solidity ^0.8.9;
pragma solidity 0.8.9;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

0.8.17

• Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.

Apart from these, there are several minor bug fixes and improvements.

2. Open TODO

The code that contains "open todos" reflects that the development is not finished and that the code can change a posteriori, prior release, with or without audit.

Affected source code:

TODO: test

• LineFactory.sol:140
• LineFactory.sol:145

3. Lack of address checks

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Also, the EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.

Reference:

• https://eips.ethereum.org/EIPS/eip-165

Affected source code:

• Oracle.sol:16
• LineFactory.sol:26
• Spigot.sol:32-34
• Escrow.sol:44-46
• EscrowedLine.sol:17
• SpigotedLine.sol:64-66
• LineOfCredit.sol:55-57

4. Don't reduce safety for gas savings

The CreditLib.accrue method says:

// interest will almost always be less than deposit // low risk of overflow unless extremely high interest rate

This low risk is not "risk free", so it doesn't make sense to have an unchecked region in this code.

  function accrue(
    ILineOfCredit.Credit memory credit,
    bytes32 id,
    address interest
  )
    public
    returns (ILineOfCredit.Credit memory)
  { unchecked {
      // interest will almost always be less than deposit
      // low risk of overflow unless extremely high interest rate

      // get token demoninated interest accrued
      uint256 accruedToken = IInterestRateCredit(interest).accrueInterest(
          id,
          credit.principal,
          credit.deposit
      );

      // update credit line balance
      credit.interestAccrued += accruedToken;

      emit InterestAccrued(id, accruedToken);
      return credit;
  } }

Affected source code:

• CreditLib.sol:247-248

5. Send ether in an un-compatible way

Because to transfer ether the .transfer method (which is capped at 2300 gas) is used instead of .call which is limited to the gas provided by the user. If a contract that has a fallback method more expensive than 2300 gas, creates an offer, it could be impossible for this contract to send the funds to the buyer or seller.

Reference:

• transfer -> The receiving smart contract should have a fallback function defined or else the transfer call will throw an error. There is a gas limit of 2300 gas, which is enough to complete the transfer operation. It is hardcoded to prevent reentrancy attacks.
• send -> It works in a similar way as to transfer call and has a gas limit of 2300 gas as well. It returns the status as a boolean.
• call -> It is the recommended way of sending ETH to a smart contract. The empty argument triggers the fallback function of the receiving address.

Affected source code:

• LineLib.sol:48

Recommended Mitigation Steps:

• Use call instead.

---

Non critical

6. Add missing @param information

Some parameters have not been commented, which reflects that the logic has been updated but not the documentation, it is advisable that developers update both at the same time to avoid the code being out of sync with the project documentation.

  /**
    * see ILineOfCredit._createCredit
    * @notice called by LineOfCredit._createCredit during every repayment function
+   * @param id - Fill me
+   * @param amount - Fill me
+   * @param lender - Fill me
+   * @param token - Fill me
    * @param oracle - interset rate contract used by line that will calculate interest owed
   */
  function create(
      bytes32 id,
      uint256 amount,
      address lender,
      address token,
      address oracle
  )

Affected source code:

• CreditLib.sol:120-131

7. Wrong return

The method removePosition present inside the library CreditListLib return true when the id was not found, something that should be fixed in order to prevent a wrong use of this method.

Affected source code:

• CreditListLib.sol:31

• Gas
  • 1. Use a recent solidity version
  • 2. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 16 = 256
  • 3. Remove empty blocks
  • 4. Reduce math operations
  • 5. Reduce the size of error messages Long revert Strings
    • Total gas saved: 18 * 1 = 18
  • 6. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 24 = 216
  • 7. Avoid unused returns
  • 8. delete optimization
    • Total gas saved: 5 * 1 = 5
  • 9. Use the unchecked keyword
  • 10. Avoid index for values

Gas

1. Use a recent solidity version

Using an updated version of solidity you can achieve significant gas savings, like the one mentioned above:

• With at least 0.8.10 to skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist.
• With at least 0.8.16 to have a more efficient code for checked addition and subtraction.
• With at least 0.8.17 to have a more efficient overflow checks for multiplication.

2. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint256 private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
Uint256 private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• LineOfCredit.sol:276
• LineOfCredit.sol:351
• SpigotedLine.sol:122
• SpigotedLine.sol:125
• SpigotedLine.sol:144
• SpigotedLine.sol:172
• CreditLib.sol:177
• CreditLib.sol:178
• CreditLib.sol:186
• CreditLib.sol:187
• CreditLib.sol:218
• CreditLib.sol:227
• CreditLib.sol:258
• EscrowLib.sol:96
• EscrowLib.sol:164
• EscrowLib.sol:202

Total gas saved: 13 * 16 = 256

3. Remove empty blocks

An empty block or an empty method generally indicates a lack of logic that it’s unnecessary and should be eliminated, call a method that literally does nothing only consumes gas unnecessarily, if it is a virtual method which is expects it to be filled by the class that implements it, it is better to use abstract contracts with just the definition.

Sample code:

pragma solidity >=0.4.0 <0.7.0;

abstract contract BaseContract {
    function totalSupply() public virtual returns (uint256);
}

Reference:

• https://docs.soliditylang.org/en/v0.6.0/contracts.html?highlight=abstract#abstract-contracts

Affected source code:

• SecuredLine.sol:13-32

4. Reduce math operations

Several methods have been found in which it is possible to reduce the number of mathematical operations, the cases are detailed below.

Affected source code:

Use scientific notation rather than exponentiation:

• 10**5=> 1e5 : EscrowLib.sol:42 and EscrowLib.sol:42

5. Reduce the size of error messages (Long revert Strings)

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.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testShortRevert(bool path) public view {
require(path, "test error");
}
}

contract TesterB {
function testLongRevert(bool path) public view {
require(path, "test big error message, more than 32 bytes");
}
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886
TesterB.testLongRevert:  21904

Affected source code:

• InterestRateCredit.sol:26-29

Total gas saved: 18 * 1 = 18

6. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• EscrowedLine.sol:64
• EscrowedLine.sol:90
• LineOfCredit.sol:112
• LineOfCredit.sol:241
• LineOfCredit.sol:259
• LineOfCredit.sol:326
• SpigotedLine.sol:62
• SpigotedLine.sol:143
• SpigotedLine.sol:160
• SpigotedLine.sol:239
• InterestRateCredit.sol:26-29
• EscrowLib.sol:91
• EscrowLib.sol:105
• EscrowLib.sol:161
• EscrowLib.sol:198
• EscrowLib.sol:216
• SpigotLib.sol:96
• SpigotLib.sol:128
• SpigotLib.sol:130
• SpigotLib.sol:155
• SpigotLib.sol:180
• SpigotLib.sol:189
• SpigotLib.sol:201
• SpigotedLineLib.sol:147

Total gas saved: 9 * 24 = 216

7. Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• LineLib.sol:34-51
• LineLib.sol:59-74
• EscrowedLine.sol:77-80
• EscrowedLine.sol:89-92
• LineOfCredit.sol:157-160
• LineOfCredit.sol:200-210
• LineOfCredit.sol:247-262
• LineOfCredit.sol:265-285
• LineOfCredit.sol:292-311
• LineOfCredit.sol:315-333
• LineOfCredit.sol:340-367
• LineOfCredit.sol:370-385
• LineOfCredit.sol:388-409
• LineOfCredit.sol:483-507
• SecuredLine.sol:48-66
• SpigotedLine.sol:137-151
• InterestRateCredit.sol:74-86
• CreditListLib.sol:20-32
• CreditListLib.sol:40-60
• EscrowLib.sol:104-149
• EscrowLib.sol:192-207
• EscrowLib.sol:215-219
• SpigotLib.sol:61-80
• SpigotLib.sol:125-140
• SpigotLib.sol:143-161
• SpigotLib.sol:164-175
• SpigotLib.sol:178-184
• SpigotLib.sol:187-193
• SpigotLib.sol:196-205
• SpigotLib.sol:208-213
• SpigotedLineLib.sol:120-140
• SpigotedLineLib.sol:146-149
• SpigotedLineLib.sol:151-160

8. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

Affected source code:

• SpigotLib.sol:117

Total gas saved: 5 * 1 = 5

9. Use the unchecked keyword

When an underflow or overflow cannot occur, one might conserve gas by using the unchecked keyword to prevent unnecessary arithmetic underflow/overflow tests.

Affected source code:

• LineOfCredit.sol:179
• LineOfCredit.sol:203
• LineOfCredit.sol:520
• CreditListLib.sol:23
• CreditListLib.sol:51
• EscrowLib.sol:57

10. Avoid index for values

There is no need to create index for amount or values, these values will be different each time, and there are no reason to have an index on them.

    event AddCredit(
        address indexed lender,
        address indexed token,
        uint256 indexed deposit,
        bytes32 id
    );

  /// @notice Emits data re Lender removes funds (principal) - there is no corresponding function, just withdraw()
  event WithdrawDeposit(bytes32 indexed id, uint256 indexed amount);
  
  /// @notice Emits data re Lender withdraws interest - there is no corresponding function, just withdraw()
  // Bob - consider changing event name to WithdrawInterest
  event WithdrawProfit(bytes32 indexed id, uint256 indexed amount);
  

  /// @notice After accrueInterest runs, emits the amount of interest added to a Borrower's outstanding balance of interest due 
  // but not yet repaid to the Line of Credit contract
  event InterestAccrued(bytes32 indexed id, uint256 indexed amount);


  // Borrower Events

  event Borrow(bytes32 indexed id, uint256 indexed amount);
  // Emits notice that a Borrower has drawn down an amount on a credit line

  event RepayInterest(bytes32 indexed id, uint256 indexed amount);
  /** Emits that a Borrower has repaid an amount of interest 
  (N.B. results in an increase in interestRepaid, i.e. interest not yet withdrawn by a Lender). There is no corresponding function
  */
  
  event RepayPrincipal(bytes32 indexed id, uint256 indexed amount);
  // Emits that a Borrower has repaid an amount of principal - there is no corresponding function

Affected source code:

• CreditLib.sol:16-48