Debt DAO contest - __141345__'s results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L483-L507

Vulnerability details

Impact

The contract fund could be drained due to reentrancy possibility in _close().

Proof of Concept

In _close() the token transfer happens before the storage state change of ids. Hence, reentrancy is possible, as long as the token has the callback hook. For example, some ERC777 token, or even existing token could have this feature after upgrade.

// Line-of-Credit/contracts/modules/credit/LineOfCredit.sol
    function _close(Credit memory credit, bytes32 id) internal virtual returns (bool) {
        if(credit.principal > 0) { revert CloseFailedWithPrincipal(); }

        // return the Lender's funds that are being repaid
        if (credit.deposit + credit.interestRepaid > 0) {
            LineLib.sendOutTokenOrETH(
                credit.token,
                credit.lender,
                credit.deposit + credit.interestRepaid
            );
        }

        delete credits[id]; // gas refunds

        // remove from active list
        ids.removePosition(id);
        unchecked { --count; }

        // ...
    }

A malicious lender can call close(id) and reenter to drain the fund, or can use another address as the borrower to call depositAndClose() to do similar thing.

Tools Used

Manual analysis.

Recommended Mitigation Steps

For the _close() function:

• add nonReentrant modifier
• follow check-effects-interaction pattern

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L59-L74

Vulnerability details

Impact

If ERC20 and eth are transferred at same time, the mistakenly sent eth will be locked. There are several functions could be affected and cause user fund lock:

• addCollateral()
• addCredit()
• increaseCredit()
• depositAndClose()
• depositAndRepay()
• close()

Proof of Concept

In receiveTokenOrETH(), different logic is used to handle ERC20 and eth transfer. However, in the ERC20 if block, mistakenly sent eth will be ignored. This part of eth will be locked in the contract.

// Line-of-Credit/contracts/utils/LineLib.sol
    function receiveTokenOrETH(
      address token,
      address sender,
      uint256 amount
    )
      external
      returns (bool)
    {
        if(token == address(0)) { revert TransferFailed(); }
        if(token != Denominations.ETH) { // ERC20
            IERC20(token).safeTransferFrom(sender, address(this), amount);
        } else { // ETH
            if(msg.value < amount) { revert TransferFailed(); }
        }
        return true;
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

In the ERC20 part, add check for msg.value to ensure no eth is sent:

        if(token != Denominations.ETH) { // ERC20
            if (msg.value > 0) { revert TransferFailed(); }
            IERC20(token).safeTransferFrom(sender, address(this), amount);
        } else { // ETH

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L94-L96 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/EscrowLib.sol#L75-L79 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L273-L280 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L487-L493

Vulnerability details

Impact

Some ERC20 may be tricky for the balance. Such as:

• fee on transfer (STA, USDT also has this mode)
• rebasing (aToken from AAVE)
• variable balance (stETH, balance could go up and down)

For these tokens, the balance can change over time, even without transfer()/transferFrom(). But current accounting stores the spot balance of the asset.

The impacts include:

• the calculation of collateral value could be inaccurate
• protocol could lose fund due to the deposit/repay amount might be less than the actual transferred amount after fee
• the amount user withdraw collateral when _close() will be inaccurate
  • some users could lose fund due to under value
  • some fund could be locked due to the balance inflation
  • some fund might be locked due to the balance deflation

Proof of Concept

The spot new deposit amount is stored in the mapping self.deposited[token].amount and credit.deposit, and later used to calculate the collateral value and withdraw amount.

// Line-of-Credit/contracts/utils/EscrowLib.sol
    function addCollateral(EscrowState storage self, address oracle, uint256 amount, address token) {
        // ...
        LineLib.receiveTokenOrETH(token, msg.sender, amount);

        self.deposited[token].amount += amount;
        // ...
    }

    function _getCollateralValue(EscrowState storage self, address oracle) public returns (uint256) {
            // ...
            d = self.deposited[token];
                // ...
                collateralValue += CreditLib.calculateValue(
                  o.getLatestAnswer(d.asset),
                  deposit,
                  d.assetDecimals
                );
            // ...
    }

// Line-of-Credit/contracts/modules/credit/LineOfCredit.sol
    function increaseCredit(bytes32 id, uint256 amount) {
        // ...
        Credit memory credit = credits[id];
        credit = _accrue(credit, id);

        credit.deposit += amount;
        
        credits[id] = credit;

        LineLib.receiveTokenOrETH(credit.token, credit.lender, amount);

        // ...
    }

    function _close(Credit memory credit, bytes32 id) internal virtual returns (bool) {
        // ...
        if (credit.deposit + credit.interestRepaid > 0) {
            LineLib.sendOutTokenOrETH(
                credit.token,
                credit.lender,
                credit.deposit + credit.interestRepaid
            );
        }

However, if the balance changed later, the returned collateral value will be inaccurate. And the amount used when withdraw collateral in _close() is also wrong.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• checking the before and after balance of token transfer
• recording the relative shares of each user instead of specific amount
• if necessary, call ERC20(token).balanceOf() to confirm the balance
• disallow such kind of tokens

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L48

Vulnerability details

Impact

When withdrawing and refund ETH, the contract uses Solidity’s transfer() function.

Using Solidity's transfer() function has some notable shortcomings when the withdrawer is a smart contract, which can render ETH deposits impossible to withdraw. Specifically, the withdrawal will inevitably fail when:

• The withdrawer smart contract does not implement a payable fallback function.
• The withdrawer smart contract implements a payable fallback function which uses more than 2300 gas units.
• The withdrawer smart contract implements a payable fallback function which needs less than 2300 gas units but is called through a proxy that raises the call’s gas usage above 2300.

Risks of reentrancy stemming from the use of this function can be mitigated by tightly following the "Check-Effects-Interactions" pattern and using OpenZeppelin Contract’s ReentrancyGuard contract. 

Proof of Concept

// Line-of-Credit/contracts/utils/LineLib.sol
48:    payable(receiver).transfer(amount);

References:

The issues with transfer() are outlined here

For further reference on why using Solidity’s transfer() is no longer recommended, refer to these articles.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised, reference.

Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

Instances number of this issue: 2

// Line-of-Credit/contracts/utils/SpigotLib.sol
    event ClaimRevenue(
        address indexed token,
        uint256 indexed amount,
        uint256 escrowed,
        address revenueContract
    );

    event ClaimEscrow(
        address indexed token,
        uint256 indexed amount,
        address owner
    );

Unused/empty receive()/fallback() function

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert (e.g. require(msg.sender == address(weth))). Having no access control on the function means that someone may send Ether to the contract, and have no way to get anything back out, which is a loss of funds

// Line-of-Credit/contracts/modules/spigot/Spigot.sol
    receive() external payable {
        return;
    }

Code Structure Deviates From Best-Practice

The best-practice layout for a contract should follow the following order:

• state variables
• events
• modifiers
• constructor
• functions

Function ordering helps readers identify which functions they can call and find constructor and fallback functions easier. Functions should be grouped according to their visibility and ordered:

• constructor
• receive function (if exists)
• fallback function (if exists)
• external
• public
• internal
• private
• within a grouping, place the view and pure functions last

Suggestion: Consider adopting recommended best-practice for code structure and layout.

require() statements should have descriptive reason strings

Instances number of this issue: 14

// Line-of-Credit/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);

// Line-of-Credit/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);

// Line-of-Credit/contracts/utils/SpigotLib.sol
128      require(revenueContract != address(this));
130      require(self.settings[revenueContract].transferOwnerFunction == bytes4(0));
180      require(newOwner != address(0));
187      require(newOperator != address(0));
201      require(newTreasury != address(0));

Missing Equivalence Checks in Setters

Description: Setter functions are missing checks to validate if the new value being set is the same as the current value already set in the contract. Such checks will showcase mismatches between on-chain and off-chain states.

Suggestion: This may hinder detecting discrepancies between on-chain and off-chain states leading to flawed assumptions of on-chain state and protocol behavior.

Instances number of this issue: 5

// Line-of-Credit/contracts/utils/SpigotLib.sol
    function updateOwnerSplit(SpigotState storage self, address revenueContract, uint8 ownerSplit)
    function updateOwner(SpigotState storage self, address newOwner) external returns (bool) {
    function updateOperator(SpigotState storage self, address newOperator) external returns (bool) {
    function updateTreasury(SpigotState storage self, address newTreasury) external returns (bool) {
    function updateWhitelistedFunction(SpigotState storage self, bytes4 func, bool allowed) external returns (bool) {

Missing Time locks

When critical parameters of systems need to be changed, it is required to broadcast the change via event emission and recommended to enforce the changes after a time-delay. This is to allow system users to be aware of such critical changes and give them an opportunity to exit or adjust their engagement with the system accordingly. None of the onlyOwner functions that change critical protocol addresses/parameters have a timelock for a time-delayed change to alert:

• users and give them a chance to engage/exit protocol if they are not agreeable to the changes
• team in case of compromised owner(s) and give them a chance to perform incident response.

Instances number of this issue: 5

// Line-of-Credit/contracts/utils/SpigotLib.sol
    function updateOwnerSplit(SpigotState storage self, address revenueContract, uint8 ownerSplit)
    function updateOwner(SpigotState storage self, address newOwner) external returns (bool) {
    function updateOperator(SpigotState storage self, address newOperator) external returns (bool) {
    function updateTreasury(SpigotState storage self, address newTreasury) external returns (bool) {
    function updateWhitelistedFunction(SpigotState storage self, bytes4 func, bool allowed) external returns (bool) {

Suggestion: Users may be surprised when critical parameters are changed. Furthermore, it can erode users' trust since they can’t be sure the protocol rules won’t be changed later on. Compromised owner keys may be used to change protocol addresses/parameters to benefit attackers. Without a time-delay, authorized owners have no time for any planned incident response. (May be medium since owner can change critical parameters at anytime that can affect the users poorly).

Events not emitted for important state changes

When changing critical state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.

Instances number of this issue: 2

// Line-of-Credit/contracts/utils/EscrowLib.sol
    function updateLine(EscrowState storage self, address _line) external returns(bool) {

// Line-of-Credit/contracts/modules/escrow/Escrow.sol
    function updateLine(address _line) external returns(bool) {

Suggestion: Consider emitting events after sensitive changes take place, to facilitate tracking and notify off-chain clients following the contract’s activity.

comment is misleading

// Line-of-Credit/contracts/modules/oracle/Oracle.sol
11
 *          - only makes request for USD prices and returns results in standard 8 decimals for Chainlink USD feeds

20
    /**
     * @return price - the latest price in USD to 8 decimals
     */
    function getLatestAnswer(address token) external returns (int) {

// Line-of-Credit/contracts/utils/CreditLib.sol
    * @dev            - Assumes Oracle returns answers in USD with 1e8 decimals
    * @param price    - The Oracle price of the asset. 8 decimals

    * @return         - The total USD value of the amount of tokens being valued in 8 decimals 

The comment indicates that the returned price is in 8 decimal, actually it is not, each feed could return different decimal.

borrow() does not follow check-effect-interaction pattern

Although it does not seem to lead to fund loss, the queue sort could potentially be messed up. And check-effect-interaction pattern is always a good practice.

// Line-of-Credit/contracts/modules/credit/LineOfCredit.sol
    function borrow(bytes32 id, uint256 amount) {
        // ...
        LineLib.sendOutTokenOrETH(credit.token, borrower, amount);
        // ...
        _sortIntoQ(id);
        // ...
    }

Suggestion: For the borrow() function:

• add nonReentrant modifier
• follow check-effects-interaction pattern

need way to remove collateral

Currently can only add new collateral, but if some assets were depreciated, they should be removed.

Suggestion: Add some method to remove the collateral.

Variable re-arrangement by storage packing

Reference: Layout of State Variables in Storage.

In struct CoreLineParams, uint256 ttl can be placed in the beginning of the struct, as a result, 1 slot storage can be saved. According to the currently layout, address borrower occupies 1 slot, uint32 cratio with uint8 revenueSplit occupy 1 slot, but after re-arrangement, the 3 can be packed into 1 slot.

// Line-of-Credit/contracts/interfaces/ILineFactory.sol
    struct CoreLineParams {
        address borrower;
        uint256 ttl;
        uint32 cratio;
        uint8 revenueSplit;
    }

can be re-arranged as:

    struct CoreLineParams {
        uint256 ttl;
        address borrower;
        uint32 cratio;
        uint8 revenueSplit;
    }

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

Instances number of this issue: 16

// Line-of-Credit/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;

// Line-of-Credit/contracts/utils/CreditLib.sol
177        credit.interestAccrued -= amount;
178        credit.interestRepaid += amount;
186        credit.principal -= principalPayment;
187        credit.interestRepaid += interest;
216        amount -= interest;
218        credit.deposit -= amount;
227        credit.interestRepaid -= amount;
258        credit.interestAccrued += accruedToken;

// Line-of-Credit/contracts/utils/EscrowLib.sol
75         collateralValue += CreditLib.calculateValue(
96         self.deposited[token].amount += amount;
164        self.deposited[token].amount -= amount;
202        self.deposited[token].amount -= amount;

Add unchecked {} for subtractions where the operands cannot underflow

Some will not underflow because of a previous code or require() or if-statement

    require(a <= b); 
    x = b - a 

->

    require(a <= b); 
    unchecked { x = b - a } 

Instances number of this issue: 4

// Line-of-Credit/contracts/utils/SpigotLib.sol
95-97
        if(claimed > escrowedAmount) {
            require(LineLib.sendOutTokenOrETH(token, self.treasury, claimed - escrowedAmount));
        }

// Line-of-Credit/contracts/utils/SpigotedLineLib.sol
100-101
        if(oldClaimTokens > newClaimTokens) {
          uint256 diff = oldClaimTokens - newClaimTokens;

105-110
          if(diff > unused) revert UsedExcessTokens(claimToken,  unused); 
          // reduce reserves by consumed amount
          else return (
            tokensBought,
            unused - diff
          );

113-116
          return (
            tokensBought,
            unused + (newClaimTokens - oldClaimTokens)
          );

accrueInterest() can skip deleted credit

Some credit was deleted when repaid. These can be skipped to avoid the useless function call.

// Line-of-Credit/contracts/modules/credit/LineOfCredit.sol
    function accrueInterest() external override returns(bool) {
        uint256 len = ids.length;
        bytes32 id;
        for (uint256 i; i < len; ++i) {
          id = ids[i];
          Credit memory credit = credits[id];
          credits[id] = _accrue(credit, id);
        }
        
        return true;
    }

accrue() visibility can be set to view

accrue() does not change the storage state, the visibility can be set to view.

// Line-of-Credit/contracts/utils/CreditLib.sol
  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;
  } }