Debt DAO contest - ajtra's results

Summary

Low

1. L01 - Missing checks for address(0x0) when assigning values to address state variables

Non Critical

2. NC01 - Floating pragma
3. NC02 - Outdated compiler version
4. NC03 - Using return name and name at the same
5. NC04 - Not using the named return variables when a function returns in anyware is confusing
6. NC05 - TODO comments means it's not a final version
7. NC06 - Code in comments
8. NC07 - Lack of @param

Low

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

Mitigation

Add check for address(0x0)

Lines in the code

Spigot.sol#L36-L38 SpigotedLine.sol#L66 Escrow.sol#L49-L51 LineOfCredit.sol#L56-L57

Non Critical

NC01 - Floating pragma

Description

The contracts have the pragma solidity directive ^0.8.9. It is recommended to specify a fixed compiler version to ensure that the bytecode produced does not vary between builds. This is especially important if you rely on bytecode-level verification of the code.

Mitigation

Lock the pragma.

Lines in the code

LineOfCredit.sol#L1 SpigotedLine.sol#L1 SecuredLine.sol#L1 Oracle.sol#L2 InterestRateCredit.sol#L1

NC02 - Outdated compiler version

Description

The project is using the solidity version 0.8.9. It's a best practice to use the latest release version. You can consult it in the following link

Mitigation

Update the solidity version to 0.8.17

Lines in the code

LineOfCredit.sol#L1 SpigotedLine.sol#L1 SecuredLine.sol#L1 EscrowedLine.sol#L1 Spigot.sol#L1 Escrow.sol#L1 Oracle.sol#L2 InterestRateCredit.sol#L1 LineFactory.sol#L1 CreditLib.sol#L1 LineLib.sol#L1 SpigotedLineLib.sol#L1 CreditListLib.sol#L1 EscrowLib.sol#L1 SpigotLib.sol#L1 MutualConsent.sol#L3 LineFactoryLib.sol#L1

NC03 - Using return name and name at the same

Description

There are many places in the code where is using the name return and after return the return name.

Example,

Mitigation

Remove return o return name

Lines in the code

SpigotLib.sol#L38-L57 SpigotLib.sol#L87-L101 SpigotLib.sol#L111-121 LineOfCredit.sol#L443-L453 CreditLib.sol#L82-L97 CreditLib.sol#L148-L160

NC04 - Not using the named return variables when a function returns in anyware is confusing

Spigot.sol#L74 Spigot.sol#L90

NC05 - TODO comments means it's not a final version

Description

There is a TODO comment inside the code what can mean that the code has not been finalished. Make sure that the code is finished before deployment in production.

Lines in the code

LineFactory.sol#L140 LineFactory.sol#L145

NC06 - Code in comments

Description

There are many places in the code with code commented. It's a good practice not to leave code in comments. Clean unnecesary comments.

Lines in the code

Oracle.sol#L24 Oracle.sol#L26-L28

NC07 - Lack of @param

Description

It's important to understand the code to add the NatSpec @param in all functions.

Lines in the code

Spigot.sol#L41 Spigot.sol#L45 Spigot.sol#L49 Spigot.sol#L70 Spigot.sol#L146 Spigot.sol#L216 Spigot.sol#L220

Index

1. I = I + (-) X is cheaper in gas cost than I += (-=) X
2. Use custom errors rather than REVERT()/REQUIRE() strings to save deployment gas
3. Require / Revert strings longer than 32 bytes cost extra gas
4. Use a more recent version of solidity
5. ABI.ENCODE() is less efficient than ABI.ENCODEPACKED()
6. Use unchecked in for/while loops when it's not possible to overflow
7. Calldata vs Memory
8. Using storage instead of memory for structs/arrays
9. Remove unused local variables
10. Declare a local variable when is going to be use

Details

1. I = I + (-) X is cheaper in gas cost than I += (-=) X

Description

In the following example (optimizer = 10000) it's possible to demostrate that I = I + X cost less gas than I += X in state variable.

contract Test_Optimization {
    uint256 a = 1;
    function Add () external returns (uint256) {
        a = a + 1;
        return a;
    }
}

contract Test_Without_Optimization {
    uint256 a = 1;
    function Add () external returns (uint256) {
        a += 1;
        return a;
    }
}

• Test_Optimization cost is 26324 gas
• Test_Without_Optimization cost is 26440 gas

With this optimization it's possible to save 116 gas

Lines in the code

LineOfCredit.sol#L192 LineOfCredit.sol#L193 LineOfCredit.sol#L276 LineOfCredit.sol#L351 SpigotedLine.sol#L122 SpigotedLine.sol#L125 SpigotedLine.sol#L144 SpigotedLine.sol#L172 CreditLib.sol#L177 CreditLib.sol#L178 CreditLib.sol#L186 CreditLib.sol#L187 CreditLib.sol#L216 CreditLib.sol#L218 CreditLib.sol#L227 CreditLib.sol#L258 EscrowLib.sol#L75 EscrowLib.sol#L96 EscrowLib.sol#L164 EscrowLib.sol#L202

2. Use custom errors rather than REVERT()/REQUIRE() strings to save deployment gas

Description

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.

Lines in the code

InterestRateCredit.sol#L26

3. Require / Revert strings longer than 32 bytes cost extra gas

Description

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas

Lines in the code

InterestRateCredit.sol#L26

4. Use a more recent version of solidity

Description

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value. Others improvements in the following link

Lines in the code

LineOfCredit.sol#L1 SpigotedLine.sol#L1 SecuredLine.sol#L1 EscrowedLine.sol#L1 Spigot.sol#L1 Escrow.sol#L1 Oracle.sol#L2 InterestRateCredit.sol#L1 LineFactory.sol#L1 CreditLib.sol#L1 LineLib.sol#L1 SpigotedLineLib.sol#L1 CreditListLib.sol#L1 EscrowLib.sol#L1 SpigotLib.sol#L1 MutualConsent.sol#L3 LineFactoryLib.sol#L1

5. ABI.ENCODE() is less efficient than ABI.ENCODEPACKED()

Description

abi.encode will apply ABI encoding rules. Therefore all elementary types are padded to 32 bytes and dynamic arrays include their length. Therefore it is possible to also decode this data again (with abi.decode) when the type are known.

abi.encodePacked will only use the only use the minimal required memory to encode the data. E.g. an address will only use 20 bytes and for dynamic arrays only the elements will be stored without length. For more info see the Solidity docs for packed mode

For the input of the keccak method it is important that you can ensure that the resulting bytes of the encoding are unique. So if you always encode the same types and arrays always have the same length then there is no problem. But if you switch the parameters that you encode or encode multiple dynamic arrays you might have conflicts.

For example:

abi.encodePacked(address(0x0000000000000000000000000000000000000001), uint(0)) encodes to the same as abi.encodePacked(uint(0x0000000000000000000000000000000000000001000000000000000000000000), address(0))

and

abi.encodePacked(uint, uint) encodes to the same as abi.encodePacked(uint, uint)

Therefore these examples will also generate the same hashes even so they are different inputs.

On the other hand you require less memory and therefore in most cases abi.encodePacked uses less gas than abi.encode.

Lines in the code

CreditLib.sol#L69

6. Use unchecked in for/while loops when it's not possible to overflow

Description

Use unchecked { i++; } or unchecked{ ++i; } when it's not possible to overflow to save gas.

Lines in the code

LineOfCredit.sol#L179 LineOfCredit.sol#L203 LineOfCredit.sol#L520 CreditListLib.sol#L23 CreditListLib.sol#L51 EscrowLib.sol#L57

7. Calldata vs Memory

Description

Use calldata instead of memory in a function parameter when you are only to read the data can save gas by storing it in calldata

Lines in the code

LineOfCredit.sol#L218 LineOfCredit.sol#L465 LineOfCredit.sol#L483 Spigot.sol#L125 SpigotLib.sol#L125

8. Using storage instead of memory for structs/arrays

Description

When retrieving data from a memory location, assigning the data to a memory variable causes all fields of the struct/array to be read from memory, resulting in a Gcoldsload (2100 gas) for each field of the struct/array. When reading fields from new memory variables, they cause an extra MLOAD instead of a cheap stack read. Rather than declaring a variable with the memory keyword, it is much cheaper to declare a variable with the storage keyword and cache all fields that need to be read again in a stack variable, because the fields actually read will only result in a Gcoldsload. The only case where the entire struct/array is read into a memory variable is when the entire struct/array is returned by a function, passed to a function that needs memory, or when the array/struct is read from another store array/struc

Lines in the code

LineOfCredit.sol#L205 LineOfCredit.sol#L257 LineOfCredit.sol#L273 LineOfCredit.sol#L323 LineOfCredit.sol#L375 LineOfCredit.sol#L389 SpigotedLine.sol#L139 InterestRateCredit.sol#L47 EscrowLib.sol#L108

9. Remove unused local variables

    function sweep(address to, address token) external nonReentrant returns (uint256) {
        uint256 amount = unusedTokens[token];
        delete unusedTokens[token];

-       bool success = SpigotedLineLib.sweep(
-          to,
-          token,
-          amount,
-          _updateStatus(_healthcheck()),
-          borrower,
-          arbiter
-        );

-       return success ? amount : 0;
+		return SpigotedLineLib.sweep(to,token,amount,_updateStatus(_healthcheck()),borrower,arbiter) ? amount : 0;
    }

SpigotedLine.sol#L259-L268

        address nonCaller = _getNonCaller(_signerOne, _signerTwo);

        // The consent hash is defined by the hash of the transaction call data and sender of msg,
        // which uniquely identifies the function, arguments, and sender.
        bytes32 expectedHash = keccak256(abi.encodePacked(msg.data, nonCaller));

MutualConsent.sol#L41-L45

-	Credit memory credit = credits[id];
-   credits[id] = _accrue(credit, id);
+   credits[id] = _accrue(credits[id], id);

LineOfCredit.sol#L205-L206 LineOfCredit.sol#L257-L258

10. Declare a local variable when is going to be use

Description

There are situations in the code where there are require or if-revert that can be executed without the declaration of some variables. That variables can be declared after those requires and save gas in some situations.

-       uint256 oldClaimTokens = LineLib.getBalance(claimToken);
        uint256 oldTargetTokens = LineLib.getBalance(targetToken);
        
        // @dev claim has to be called after we get balance
        // reverts if there are no tokens to claim
        uint256 claimed = ISpigot(spigot).claimEscrow(claimToken);

        trade(
            claimed + unused,
            claimToken,
            swapTarget,
            zeroExTradeData
        );
        
        // underflow revert ensures we have more tokens than we started with
        uint256 tokensBought = LineLib.getBalance(targetToken) - oldTargetTokens;

        if(tokensBought == 0) { revert TradeFailed(); } // ensure tokens bought

        uint256 newClaimTokens = LineLib.getBalance(claimToken);

        // ideally we could use oracle to calculate # of tokens to receive
        // but sellToken might not have oracle. buyToken must have oracle

        emit TradeSpigotRevenue(
            claimToken,
            claimed,
            targetToken,
            tokensBought
        );

+       uint256 oldClaimTokens = LineLib.getBalance(claimToken);
        // used reserve revenue to repay debt
        if(oldClaimTokens > newClaimTokens) {

SpigotedLineLib.sol#L68-L100