Debt DAO contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In LineOfCredit contract, both functions addCredit(...) and increaseCredit(...) require mutual consent between lender and borrower. If lender is tricked by borrower, or by mistake, lender ETH will be locked in the contract forever.

function addCredit(
    uint128 drate,
    uint128 frate,
    uint256 amount,
    address token,
    address lender
)
    external
    payable
    override
    whileActive
    mutualConsent(lender, borrower) // @audit Lender call first will lose ETH
    returns (bytes32)

Proof of Concept

The way modifier mutualConsent works for 2 entities is:

1. First call: record the context to a mapping and return but do nothing else.
2. Later call: Check the mapping and execute the logic.

modifier mutualConsent(address _signerOne, address _signerTwo) {
  if(_mutualConsent(_signerOne, _signerTwo))  {
    // Run whatever code needed 2/2 consent
    _;
  }
}

function _mutualConsent(address _signerOne, address _signerTwo) internal returns(bool) {
    if(msg.sender != _signerOne && msg.sender != _signerTwo) { revert Unauthorized(); }

    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));

    if (!mutualConsents[expectedHash]) {
        bytes32 newHash = keccak256(abi.encodePacked(msg.data, msg.sender));

        mutualConsents[newHash] = true;

        emit MutualConsentRegistered(newHash);

        return false;
    }

    delete mutualConsents[expectedHash];

    return true;
}

In case addCredit(...) with native token ETH, lenders should be the one who send ETH along their transactions (lender lend to borrower). Function receiveTokenOrETH will check if caller send enough ETH

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;
}

Applying to the logic of mutualConsent above, there are 2 cases:

1. Borrower call first, Lender call later
   • Everything is normal.
2. Lender call first, Borrower call later.
   • Lender sent ETH but no logic is executed, no credit is created.
   • Borrower did not send ETH obviously, function receiveTokenOrETH() will fail and TX is reverted.

Since these agreements are settled off-chain, there are always some human-errors to happen.

In addition, if borrower tricks lender into thinking that borrower did send the TX but actually let the lender be the first caller, ETH lender sent will be locked.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding check that sender is actualy msg.sender in receiveTokenOrETH() function

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 || msg.sender != sender) { revert TransferFailed(); }
    }
    return true;
}

Judge has assessed an item in Issue #366 as M risk. The relevant finding follows:

5. Cannot cancel mutual consent https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L11

Mutual consent works by using two TXs with the same msg.data. However, when first one call, there is no way to cancel it. First caller might send wrong msg.data or later caller change the mind in the midway.

Since it's not possible to cancel the process, later caller can take benefit and call it in the future.

Recommendation Consider allowing to cancel the mutual consent process after some time interval.

Lines of code

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

Vulnerability details

Impact

Since the interest is updated every seconds, when borrower want to repay an ETH debt, he cannot calculate exactly the amount of ETH to send.

However, these excess ETH is not sent back to borrower but locked in the contract.

Proof of Concept

Function receiveTokenOrETH() accepts more ETH than needed but not sent back to sender

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(); } // @audit can send more than needed
    }
    return true;
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider sending excess ETH to sender or adding a sweep function for LineOfCredit.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L111

Vulnerability details

Impact

Function SpigotedLine.claimAndRepay() is callable by both borrower and lender. Callers will provide zeroExTradeData to help calling trade function in 0x.

Since it's repay function, borrower has no incentive to act malicious but lender has. Lender can call with trade with no slippage protection, creating front-run opportunity, also lower the revenue from spigot.

Proof of Concept

Lender can call claimAndRepay() with arbitrary trade data

function claimAndRepay(address claimToken, bytes calldata zeroExTradeData)
    external
    whileBorrowing
    nonReentrant
    returns (uint256)
{
    bytes32 id = ids[0];
    Credit memory credit = _accrue(credits[id], id);

    if (msg.sender != borrower && msg.sender != credit.lender) {
        revert CallerAccessDenied();
    }

    uint256 newTokens = claimToken == credit.token ?
      spigot.claimEscrow(claimToken) :  // same asset. dont trade
      _claimAndTrade(                   // trade revenue token for debt obligation
          claimToken,
          credit.token,
          zeroExTradeData // @audit lender can call with no slippage protection
      );

    uint256 repaid = newTokens + unusedTokens[credit.token];
    uint256 debt = credit.interestAccrued + credit.principal;

    // cap payment to debt value
    if (repaid > debt) repaid = debt;
    // update unused amount based on usage
    if (repaid > newTokens) {
        // using bought + unused to repay line
        unusedTokens[credit.token] -= repaid - newTokens;
    } else {
        // high revenue and bought more than we need
        unusedTokens[credit.token] += newTokens - repaid;
    }

    credits[id] = _repay(credit, id, repaid);

    emit RevenuePayment(claimToken, repaid);

    return newTokens;
}

And there is no slippage protection for the trade https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/SpigotedLineLib.sol#L85

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

Tools Used

Manual Review

Recommended Mitigation Steps

Adding an Oracle to calculate the slippage is overcomplicated but still a solution. Or consider only allowing lender to claim but not trade revenue from spigot.

Judge has assessed an item in Issue #366 as M risk. The relevant finding follows:

1. Not support fee-on-transfer tokens https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L223

Every tokens of credit line will be transferred from lender to LineOfCredit first, then to borrower later. These 2-transfer steps will make the tax for some fee-on-transfer tokens accounted twice.

However, current logic is not handled it. So if lender uses fee-on-transfer tokens, contract will not have enough balance to transfer to borrower.

function addCredit( uint128 drate, uint128 frate, uint256 amount, address token, address lender ) external payable override whileActive mutualConsent(lender, borrower) // @note Lender call first will lose ETH returns (bytes32) { LineLib.receiveTokenOrETH(token, lender, amount);

bytes32 id = _createCredit(lender, token, amount);

require(interestRate.setRate(id, drate, frate));

return id;

} Recommendation Consider comparing pre and after balance when receiving token.

Lines of code

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

Vulnerability details

Impact

Sending ETH using transfer() function might block contract from receiving ETH. It forwards a fixed gas amount, and gas cost might change in the future too. So if contract does some actions in receive() function like require, assert, the transfer can fail because of out of gas.

Proof of Concept

Function sendOutTokenOrETH() used transfer() when sending ETH

function sendOutTokenOrETH(
  address token,
  address receiver,
  uint256 amount
)
  external
  returns (bool)
{
    if(token == address(0)) { revert TransferFailed(); }

    // both branches revert if call failed
    if(token!= Denominations.ETH) { // ERC20
        IERC20(token).safeTransfer(receiver, amount);
    } else { // ETH
        payable(receiver).transfer(amount); // @audit use call instead of transfer
    }
    return true;
}

Please check out the article for more information https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using call() instead of transfer() when sending ETH

Lines of code

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

Vulnerability details

Impact

LineOfCredit is considered liquidatable when all credit lines are not closed by deadline. An lender can easily reject receiving payment (ETH or tokens with callback), blocking credit line from closing and effectively making LineOfCredit liquidatable.

In addition, SecuredLine is inherited from LineOfCredit. When SecuredLine is liquidatable, it will send tokens to arbiter for OTC sales which introduce loss of funds for borrower.

Proof of Concept

Function LineOfCredit._healthcheck() return LIQUIDATABLE in case timestamp pass the deadline and count > 0. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L121-L139

function _healthcheck() internal virtual returns (LineLib.STATUS) {
    // if line is in a final end state then do not run _healthcheck()
    LineLib.STATUS s = status;
    if (
        s == LineLib.STATUS.REPAID ||               // end state - good
        s == LineLib.STATUS.INSOLVENT               // end state - bad
    ) {
        return s;
    }

    // Liquidate if all credit lines aren't closed by deadline
    if (block.timestamp >= deadline && count > 0) {
        emit Default(ids[0]); // can query all defaulted positions offchain once event picked up
        return LineLib.STATUS.LIQUIDATABLE;
    }

    // if nothing wrong, return to healthy ACTIVE state  
    return LineLib.STATUS.ACTIVE;
}

Then when SecuredLine inherits LineOfCredit, _healthcheck() will return value from LineOfCredit if that value is not ACTIVE https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SecuredLine.sol#L97-L104

function _healthcheck() internal override(EscrowedLine, LineOfCredit) returns(LineLib.STATUS) {
    LineLib.STATUS s = LineOfCredit._healthcheck();
    if(s != LineLib.STATUS.ACTIVE) {
          return s;
    }

    return EscrowedLine._healthcheck();
}

However in order to remove credit line from storage, it has to call _close() function. Value of count is only decreased here. https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L483

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(// @audit lender can reject receive token, make it liquidated ?
            credit.token,
            credit.lender,
            credit.deposit + credit.interestRepaid
        );
    }

    delete credits[id]; // gas refunds

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

    // If all credit lines are closed the the overall Line of Credit facility is declared 'repaid'.
    if (count == 0) { _updateStatus(LineLib.STATUS.REPAID); }

    emit CloseCreditPosition(id);

    return true;
}

In the function _close(), it return the Lender's funds that are being repaid. But if credit token is ETH or tokens with callback, receiver can reject receiving them (revert on function receive() for ETH), it makes that credit line cannot be closed and count always larger than 0.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider applying Pull over Push pattern when returning Lender's funds.

Summary

1. Not support fee-on-transfer tokens

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

Every tokens of credit line will be transferred from lender to LineOfCredit first, then to borrower later. These 2-transfer steps will make the tax for some fee-on-transfer tokens accounted twice.

However, current logic is not handled it. So if lender uses fee-on-transfer tokens, contract will not have enough balance to transfer to borrower.

function addCredit(
    uint128 drate,
    uint128 frate,
    uint256 amount,
    address token,
    address lender
)
    external
    payable
    override
    whileActive
    mutualConsent(lender, borrower) // @note Lender call first will lose ETH
    returns (bytes32)
{
    LineLib.receiveTokenOrETH(token, lender, amount);

    bytes32 id = _createCredit(lender, token, amount);

    require(interestRate.setRate(id, drate, frate));

    return id;
}

Recommendation

Consider comparing pre and after balance when receiving token.

2. Lender call close() will lose ETH

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

Function LineOfCredit.close() allowed both borrower and lender of the credit line to call. However, if credit token is native token, lender call this function will lose ETH but there is no check for that.

Recommendation

Consider preventing lender from calling close() if credit token is ETH

3. Implementation mismatch documentation in updateSplit() function

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

In the NatSpec comment of function updateSplit(), it said that this function is called by arbiter and borrower only. However, there is no check for that in the implementation, anyone can call this function.

However I did not found any risk here since the values to be updated are fixed (split = defaultSplit / MAX_SPLIT)

Recommendation

Consider updating the document.

4. Unsafe cast from bytes to bytes4 in function operate()

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

In operate() function, bytes data is unsafe casted to bytes4 to get the function selector. However, this unsafe cast can lead to unexpected behaviour when providing wrong data.

If bytes data is less than 4 bytes, it will pad 0 at the end when casting to bytes4. And if it is occasionally identical to another whitelisted function, it will make the call to unwhitelisted function possible (fallback(), receive()) and lead to unexpected behaviour.

Recommendation

Consider using SafeCast library

5. Cannot cancel mutual consent

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

Mutual consent works by using two TXs with the same msg.data. However, when first one call, there is no way to cancel it. First caller might send wrong msg.data or later caller change the mind in the midway.

Since it's not possible to cancel the process, later caller can take benefit and call it in the future.

Recommendation

Consider allowing to cancel the mutual consent process after some time interval.