Debt DAO contest - perseverancesuccess's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L388 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L396-L406 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L472 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/CreditListLib.sol#L40-L60

Vulnerability details

Impact

Borrower can steal lenders' money by using the close(id) function in LineOfCredit contract

In the design of the protocol, the borrower can use the function close(id) to close an existed credit ID

 function close(bytes32 id) external payable override returns (bool) {
     ...

    // ensure all money owed is accounted for. Accrue facility fee since prinicpal was paid off
        credit = _accrue(credit, id);
        uint256 facilityFee = credit.interestAccrued;
        if(facilityFee > 0) {
          // only allow repaying interest since they are skipping repayment queue.
          // If principal still owed, _close() MUST fail
          LineLib.receiveTokenOrETH(credit.token, b, facilityFee);

          credit = _repay(credit, id, facilityFee);
        }

        _close(credit, id); // deleted; no need to save to storage

         return true;
}
...

So in this case, the borrower can close any ID in the IDs queue. Since the protocol charge interest for even the credit with zero principal so if a credit ID with deposit > 0 and principal = 0 after some time e.g. 5 mintues after the lender deposit, then facilityFee > 0, then the contract receives the fee that is very small, and repay the credit.

function _repay(Credit memory credit, bytes32 id, uint256 amount)
        internal
        returns (Credit memory)
    { 
        credit = CreditLib.repay(credit, id, amount);

        // if credit line fully repaid then remove it from the repayment queue
        if (credit.principal == 0) ids.stepQ();

        return credit;
    }

If the principal is 0, then ids.StepQ() is called. In this function, the first id is removed, and all the remained ids are shifted left in ids array. So the first ids that can contain the credit information of other lender is removed from the ids queue that cause the lender to loose all the deposit money. in the case, that the borrower already borrows all the deposit, then the borrower does not need to repay.

function stepQ(bytes32[] storage ids) external returns(bool) {
      uint256 len = ids.length ;
      if(len <= 1) return true; // already ordered

      bytes32 last = ids[0];
      
      if(len == 2) {
        ids[0] = ids[1];
        ids[1] = last;
      } else {
        // move all existing ids up in the queue
        for(uint i = 1; i < len; ++i) {
          ids[i - 1] = ids[i]; // could also clean arr here like in _SortIntoQ
          
        }
        // cycle first id back to end of queue
        ids[len - 1] = last;
      }
      
      return true;
    }

Proof of Concept

The following code prove the bug Step 0: add 3 credits: credit for Token1 Lender1 , add credit for Token1 Lender2, add credit for Token2 Lender1 Step 1: The borrower borrow for id 0 and id 2. Note that after this step, the IDs array is updated Print out the Credit Values of IDs Step 2: Wait for some time e.g. 5 minutes after deposit to have the interestAccrued > 0 Step 3: Call the close(id) function to close the Id 2 that is the id with principal = 0 Step 3: Get the values of Credit IDs to verify that Id0 was removed. ID2 is also removed.

Log files: Running 1 test for contracts/tests/LineOfCredit.t.modified.sol:LineTest [PASS] test_can_close_multiple_ids() (gas: 744191) Logs: Step 0: add credit for Token1 Lender1 Step 0: add credit for Token1 Lender2 Step 0: add credit for Token2 Lender1 Step 1: The borrower borrow for id 0 Step 1: The borrower borrow for id 2 Step 1: Values of Credit[0] For ID 0: deposit: 500000000000000000000 For ID 0: principal: 500000000000000000000 For ID 0: interestAccrued: 0 For ID 0: interestRepaid: 0 Step 1: Values of Credit[1] For ID 1: deposit: 300000000000000000000 For ID 1: principal: 300000000000000000000 For ID 1: interestAccrued: 0 For ID 1: interestRepaid: 0 Step 1: Values of Credit[2] For ID 2: deposit: 100000000000000000000 For ID 2: principal: 0 For ID 2: interestAccrued: 0 For ID 2: interestRepaid: 0 Step 2: Wait for some time e.g. 5 minutes after deposit to have the interestAccrued > 0 Step 2: Values of Credit[2] For ID 2: deposit: 100000000000000000000 For ID 2: principal: 0 For ID 2: interestAccrued: 0 For ID 2: interestRepaid: 0 Step 3: Call the close(id) function to close the Id 2 Step 3: count: 2 ids.length: 3 Step 3: Values of Credit[0] For ID 0: deposit: 300000000000000000000 For ID 0: principal: 300000000000000000000 For ID 0: interestAccrued: 0 For ID 0: interestRepaid: 0 Step 3: Values of Credit[1] For ID 1: deposit: 0 For ID 1: principal: 0 For ID 1: interestAccrued: 0 For ID 1: interestRepaid: 0

You can run the POC by calling:

forge test -m test_can_close_multiple_ids -vvvvv

You can add the test case LineOfCredit.t.modified.sol in the tests folder: https://drive.google.com/file/d/1ytj7YIH6jVussaujHqv8wzw3WeDbE_hF/view?usp=share_link

You can find the detailed log file: https://drive.google.com/file/d/1f_qQc39aGnuValTLCb9KSl_0prQwSpre/view?usp=share_link

You can use the patch by apply it: the patch changed vs commit: e8aa08b44f6132a5ed901f8daa231700c5afeb3a
patch: https://drive.google.com/file/d/1xzcDCx63RBr62j2cbVmgavC6AMOYz5Dp/view?usp=share_link

git apply POC.patch

Or Full Repo: https://drive.google.com/file/d/1gemu5RSPD354-jKzbM7N-Dluai4hGn7j/view?usp=share_link

Tools Used

Foundry

Recommended Mitigation Steps

The repay in close(id) only repay the interestAccrued so the logic for repay in the closed(id) should take this scenario into consideration to have correct logic code.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L265 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L223 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L315 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L71

Vulnerability details

Impact

Lender or borrower can loose money if mistakenly sends more than the amount of ETH than the amount money

In the design of the protocol, the lender can use the function increaseCredit or addCredit to add the deposit money for the credit. Or the borrower can use the function depositAndRepay to deposit and repay for the credit

function addCredit(
        uint128 drate,
        uint128 frate,
        uint256 amount,
        address token,
        address lender
    )
        external
        payable
        override
        whileActive
        mutualConsent(lender, borrower)
        returns (bytes32)
    {
      ...
     
      LineLib.receiveTokenOrETH(token, lender, amount);
      ...
    }

function increaseCredit(bytes32 id, uint256 amount)
      external
      payable
      override
      whileActive
      mutualConsentById(id)
      returns (bool)
    {
      ...
     
     LineLib.receiveTokenOrETH(credit.token, credit.lender, amount);
      ...
    }

function depositAndRepay(uint256 amount)
        external
        payable
        override
        whileBorrowing
        returns (bool)
    {
      ...
      LineLib.receiveTokenOrETH(credit.token, msg.sender, amount);
      ...
    }

All these above functions use the amount as input parameter. So after the borrower and lender reaches agreement on the amount, the lender can call increaseCredit or addCredit. and the borrower can use depositAndRepay always to repay.

But the function LineLib.receiveTokenOrETH(credit.token, msg.sender, amount) just check if the msg.value < amount.

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

So in case of using ETH, msg.value can be > amount then the transaction still success. But in this case, the lender or the borrower loose money, because the excessed amount (msg.value - amount) is not tracked anywhere in the protocol.

Proof of Concept

The following scenarios is possible and cause users loose money

Scenario 1:

Step 0: The borrower call addCredit or increaseCredit with amount Step 1: The lender call addCredit or increaseCredit with amount but the msg.value is > amount The transaction success. The lender lost some money that is (msg.value - amount).

Scenario 2:

Step 0: The credit was created. the borrower has borrowed some money. Step 1: The borrower call depositAndRepay and sent the msg.value > amount . Then the borrower lost money (msg.value - amount).

Tools Used

VSCode

Recommended Mitigation Steps

Should check

if(msg.value != amount) { revert TransferFailed(); }

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L106-L112 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotedLineLib.sol#L75-L85

Vulnerability details

Impact

Lender can trade claimToken in a malicious way to steal the borrower's money via claimAndRepay() in SpigotedLine by using malicious zeroExTradeData.

In the design of the protocol, the lender can use the function claimAndRepay(), the lender can take claimToken by spigot.claimEscrow and then trade the claimToken to the CreditTOken via ZeroEx exchange, then repay the credit.

function claimAndRepay(address claimToken, bytes calldata zeroExTradeData) external
        whileBorrowing
        nonReentrant
        returns (uint256) { 

...
// Line 106 - Line 112
uint256 newTokens = claimToken == credit.token ?
          spigot.claimEscrow(claimToken) :  // same asset. dont trade
          _claimAndTrade(                   // trade revenue token for debt obligation
              claimToken,
              credit.token,
              zeroExTradeData
          );
...
// Line 128 - Line 130 
 credits[id] = _repay(credit, id, repaid);

        emit RevenuePayment(claimToken, repaid);

...

}

function _claimAndTrade(
      address claimToken,
      address targetToken,
      bytes calldata zeroExTradeData
    )
        internal
        returns (uint256)
    {
        (uint256 tokensBought, uint256 totalUnused) = SpigotedLineLib.claimAndTrade(
            claimToken,
            targetToken,
            swapTarget,
            address(spigot),
            unusedTokens[claimToken],
            zeroExTradeData
        );

        // we dont use revenue after this so can store now
        unusedTokens[claimToken] = totalUnused;
        return tokensBought;
    }

function claimAndTrade(
        address claimToken,
        address targetToken,
        address payable swapTarget,
        address spigot,
        uint256 unused,
        bytes calldata zeroExTradeData
    )
    external 
        returns(uint256, uint256)

{
...
 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
...


}

In the function to claimAndTrade in SpigotedLineLib.sol, the check in line 85 to check if tokenBought is not equal to 0 then revert.

The bug here is the zeroExTradeData is controlled by the lender and can be malicious and can manipulate the flow to bypass the check in line 85.

Proof of Concept

The following code can manipulate and bypass the check to steal money of the borrower. Step 1: Construct the zeroExTradeData data to sell the claimToken to ETH via the ZeroEx exchange data. The lender constructs the zeroExTradeData to send ETH to the exploit contract. Step 2: In the exploit contract, have the receive() function to receive ETH from ZeroEx exchange. Since the exchange was from claimToken to ETH, so the exploit contract will receive the ETH and the code in receive function will be hit.

receive() external payable {
    console.log("Callback hit: Send the SpigottedLine Contract some CreditToken to bypass the check of Balance");
    uint256 amount = 100; 
    creditToken.transfer(address(line),amount);
    console.log("Receive the amount of ETH: %s", msg.value);
  }

In the receive() function, the exploit contract transfer some amount of creditToken to the SpigotedLine contract to bypass the check

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

Since this check requires only not 0, so the lender can send only 1 or very small amount, e.g. 100 of creditToken.

This amount then will be used to repay the credit. So this means, the borrower lost money, because the lender can claim big amount of claimToken and repay a little for the credit.

In the zip file in the Google_Drive link, there is the POC written for this bug. The test case is test_lender_can_claim_and_repay_3 in file SpigotedLine.t.modified.sol You can put this file to the tests folder https://drive.google.com/file/d/1IWAV8Zz5KVgw22-gnVZrOxkcYrgv8cO2/view?usp=sharing

You can run the POC by calling:

forge test -m test_lender_can_claim_and_repay_3 -vvvvv --fork-url 'https://mainnet.infura.io/v3/61b30ad3285446cf86bed0c053d864af' --fork-block-n
umber 15918000

Here I use the block-number to make the test log stable, but this does not impact the logic of POC.

You can find the detailed log file: Line-of-Credit\test_claim_221107_2311.log The full log file here: https://drive.google.com/file/d/1LTY2-z8gOIOen0Ut9CbX1KpwvDvNVQdx/view?usp=sharing In this log file, the lender claims 1000 DAI (DAI is revenueToken) then sell to receive 0.6324 ETH, but repays only 100 * ( 10 ** -18 ) BUSD for the borrower.

Logs: Step 0: As a Borrower borrow some money Step 1: Construct the tradeData to call claimAndRepay as the lender claimed: 1000000000000000000000 unused: 0 sellAmount: 1000000000000000000000 Step 1: As the lender, call claimAndRepay with Malicious zeroExTradeData Callback hit: Send the SpigottedLine Contract some CreditToken to bypass the check of Balance Receive the amount of ETH: 632428006785336734 emit RepayInterest(id: 0xa874d902851500473943ebb58b0c06aca6125454fa55abe5637379305db10141, amount: 0) emit RepayPrincipal(id: 0xa874d902851500473943ebb58b0c06aca6125454fa55abe5637379305db10141, amount: 100) RevenuePayment(token: DAI: [0x6b175474e89094c44da98b954eedeac495271d0f], amount: 100)

You can use the POC.patch here: https://drive.google.com/file/d/17Ycdi5czBoFOKNQlgVqWxVdHxfw04304/view?usp=sharing To use it use command

git apply POC.patch

To run use command

forge install
forge test -m test_lender_can_claim_and_repay_3 -vvvvv --fork-url 'https://mainnet.infura.io/v3/61b30ad3285446cf86bed0c053d864af' --fork-block-n
umber 15918000

The full code repository: https://drive.google.com/file/d/1LTY2-z8gOIOen0Ut9CbX1KpwvDvNVQdx/view?usp=sharing

Tools Used

Foundry

Recommended Mitigation Steps

This is a difficult bug to fix if the protocol still allows the lender to use this functionality. Probably should limit this functionality for the borrower to use. Because the borrower will not benefit from stealing his own money.