Debt DAO contest - Aymen0909's results

QA Report

Summary

Findings

1- Use call() instead of transfer() :

When transferring ETH, call() should be used instead of transfer().

The transfer() function only allows the recipient to use 2300 gas. If the recipient uses more than that, transfers will fail. In the future gas costs might change increasing the likelihood of that happening.

Keep in mind that call() introduces the risk of reentrancy, but as the code follows the CEI pattern it should be fine.

Risk : Low

Proof of Concept

Instances include:

File: contracts/utils/LineLib.sol Line 48

payable(receiver).transfer(amount);

Mitigation

Replace transfer() calls with call(), keep in mind to check whether the call was successful by validating the return value.

2- Chainlink latestRoundData() could return stale prices :

The Chainlink latestRoundData() function can sometimes return old prices if the current round is not complete or an old round is used, this can have a negative impact on the protocol lending/borrwing operations as the oracle is used to determine the assets prices when trying to trade or withdraw liquidity.

Risk : Low

Proof of Concept

Instances include:

File: contracts/modules/oracle/Oracle.sol Line 22-32

function getLatestAnswer(address token) external returns (int) {
    (
        /* uint80 roundID */, 
        int price,
        /* uint80 startedAt */,
        /* uint80 timeStamp */,
        /* uint80 answeredInRound */
    ) = registry.latestRoundData(token, Denominations.USD);
    
    return price;
}

Mitigation

To avoid the risk of getting stale price from Chainlink oracle i recommend to use the latestRoundData function with it's required checks.

The functions getLatestAnswer should be modified as follow :

function getLatestAnswer(address token) external returns (int) {
    (
        uint80 roundID ,
        uint price, 
        uint256 timestamp, 
        uint80 answeredInRound
    ) = registry.latestRoundData(token, Denominations.USD);

    require(price > 0, "Invalid feed price");
    require(answeredInRound >= roundID, "Stale price");
    require(timestamp != 0, "Round not complete");
    
    return price;
}

3- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address, uint, int) is not the zero value (address(0) or 0)

Risk : Low

Proof of Concept

Instances include:

File: contracts/modules/credit/EscrowedLine.sol Line 17

escrow = IEscrow(_escrow);

File: contracts/modules/credit/LineOfCredit.sol Line 55

oracle = IOracle(oracle_);

File: contracts/modules/credit/LineOfCredit.sol Line 56

arbiter = arbiter_;

File: contracts/modules/credit/LineOfCredit.sol Line 57

borrower = borrower_;

File: contracts/modules/credit/SpigotedLine.sol Line 64

spigot = ISpigot(spigot_);

File: contracts/modules/credit/SpigotedLine.sol Line 66

swapTarget = swapTarget_;

File: contracts/modules/escrow/Escrow.sol Line 49

oracle = _oracle;

File: contracts/modules/escrow/Escrow.sol Line 50

borrower = _borrower;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

4- require()/revert() statements should have descriptive reason strings :

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/modules/credit/LineOfCredit.sol

112     require(uint(status) >= uint( LineLib.STATUS.ACTIVE));
241     require(interestRate.setRate(id, drate, frate));
259     require(interestRate.setRate(id, drate, frate));
326     require(amount <= credit.principal + credit.interestAccrued);

File: contracts/modules/credit/SpigotedLine.sol

62      require(defaultRevenueSplit_ <= SpigotedLineLib.MAX_SPLIT);
143     require(amount <= unusedTokens[credit.token]);
160     require(msg.sender == borrower);
239     require(msg.sender == arbiter);

File: contracts/utils/EscrowLib.sol

91      require(amount > 0);
105     require(msg.sender == ILineOfCredit(self.line).arbiter());
161     require(amount > 0);
198     require(amount > 0);
216     require(msg.sender == self.line);

File: contracts/utils/SpigotLib.sol

128     require(revenueContract != address(this));
130     require(self.settings[revenueContract].transferOwnerFunction == bytes4(0));
155     require(success);
180     require(newOwner != address(0));
189     require(newOperator != address(0));
201     require(newTreasury != address(0));

Mitigation

Add reason strings to the aforementioned require statements for better comprehension.

5- Duplicated require()/revert() checks should be refactored to a modifier :

Risk : NON CRITICAL

Proof of Concept

There are 6 instances of this issue:

File: contracts/utils/SpigotLib.sol

109     if(msg.sender != self.owner) { revert CallerAccessDenied(); }     
126     if(msg.sender != self.owner) { revert CallerAccessDenied(); }
147     if(msg.sender != self.owner) { revert CallerAccessDenied(); }
168     if(msg.sender != self.owner) { revert CallerAccessDenied(); }
179     if(msg.sender != self.owner) { revert CallerAccessDenied(); }
209     if(msg.sender != self.owner) { revert CallerAccessDenied(); }

Mitigation

Those checks should be replaced by a onlyOwner modifier as follow :

modifier onlyOwner(){
    if(msg.sender != self.owner) { revert CallerAccessDenied(); } 
    _;
}

6- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/utils/CreditLib.sol

returns (ILineOfCredit.Credit memory c, uint256 principal, uint256 interest)

returns(ILineOfCredit.Credit memory credit)

File: contracts/utils/SpigotLib.sol

returns (uint256 claimed)

returns (uint256 claimed)

returns (uint256 claimed)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

Gas Optimizations

Summary

Findings

1- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 3 instances of this issue:

File: contracts/modules/credit/SpigotedLine.sol Line 144

unusedTokens[credit.token] -= amount;

The above operation cannot underflow due to the check : require(amount <= unusedTokens[credit.token]); and should be modified as follows :

unchecked {
    unusedTokens[credit.token] -= amount;
}

File: contracts/utils/EscrowLib.sol Line 164

self.deposited[token].amount -= amount;

The above operation cannot underflow due to the check : if(self.deposited[token].amount < amount) { revert InvalidCollateral(); } and should be modified as follows :

unchecked {
    self.deposited[token].amount -= amount;
}

File: contracts/utils/EscrowLib.sol Line 202

self.deposited[token].amount -= amount;

The above operation cannot underflow due to the check : if(self.deposited[token].amount < amount) { revert InvalidCollateral(); } and should be modified as follows :

unchecked {
    self.deposited[token].amount -= amount;
}

2- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 7 instances of this issue:

File: contracts/modules/credit/SpigotedLine.sol

122     unusedTokens[credit.token] -= repaid - newTokens;
125     unusedTokens[credit.token] += newTokens - repaid;
144     unusedTokens[credit.token] -= amount;
172     unusedTokens[targetToken] += newTokens;

File: contracts/utils/EscrowLib.sol

96      self.deposited[token].amount += amount;
164     self.deposited[token].amount -= amount;
202     self.deposited[token].amount -= amount;

3- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 5 instances of this issue:

File: contracts/modules/credit/LineOfCredit.sol

179     for (uint256 i; i < len; ++i)
203     for (uint256 i; i < len; ++i)
520     for (uint256 i; i <= lastSpot; ++i)

File: contracts/utils/CreditListLib.sol

23      for(uint256 i; i < len; ++i)
51      for(uint i = 1; i < len; ++i)