Debt DAO contest - joestakey's results

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

L02 - _close() should not be able to close a specific id credit line As per the docs:

Can a Borrower chose to repay any debt in any order? No. The app automatically selects which credit line can be repaid using a first-in-first-out (FIFO) repayment queue based on the order in which credit lines are drawn down The issue is that close() takes an id argument, meaning it technically breaks this protocol requirement if called for any id other than ids[0]. Now, because of this credit.principal check, it is actually impossible to close any id other than the first one - because it is only possible to repay the full debt through depositAndClose and depositAndRepay, which always repay the first id in the array ids.

close() can hence only work for id = ids[0] anyway.

Because of the protocol desire for it to be composable, this can open risks if a child contract inheriting LineOfCredit were to give the possibility to repay full debt of any id credit line. This would mean a borrower could repay and close in any order.

Impact Low

Tools Used Manual Analysis

Mitigation Modify both close() and _close(), so that they can only close the first credit line in queue

-388: function close(bytes32 id) external payable override returns (bool) { +388: function close() external payable override returns (bool) {

•        bytes32 id = ids[0];
  

389: Credit memory credit = credits[id]; ... -406: _close(credit, id); +406: _close(credit); -483: function _close(Credit memory credit, bytes32 id) internal virtual returns (bool) { +483: function _close(Credit memory credit) internal virtual returns (bool) {

•        bytes32 id = ids[0];
  

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L237 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L280 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L71

Vulnerability details

When a lender wants to create a credit line or add credit to an existing credit line, the LineLib.receiveTokenOrETH method is used. If the token is ETH, the call reverts if msg.value is less than amount. But it does not revert if msg.value is strictly greater than amount. If a lender passes msg.value > amount while adding credit, msg.value - amount will be lost, as there is no function in LineOfCredit() or LineLib to recover ETH.

Note that there is indeed a sendOutTokenOrETH method that transfers ETH out of the contract, however:

• In borrow(), the call would revert here, as that extra msg.value - amount was not taken into account in credit.deposit
• In withdraw(), the call would revert here, again because that extra msg.value - amount was not taken into account in credit.deposit
• In depositAndClose(), there is no such checks and the call would go through. But as it is repayment, it means a borrower would only need to transfer credit.deposit + credit.interestRepaid - lostMsgValue into the contract, then call close() to close the credit line, repaying the lender. So while the lender retrieved that lostMsgValue, it was used by the borrower during repayment. (lostMsgValue being msg.value - amount).

Therefore in any case, that msg.value - amount is lost to the lender.

Note that this library function is also used in other contracts, such as Escrow to add collateral.

Impact

Medium

Tools Used

Manual Analysis

Mitigation

The msg.value check should ensure it is equal to amount.

67:         if(token == address(0)) { revert TransferFailed(); }
68:         if(token != Denominations.ETH) { // ERC20
69:             IERC20(token).safeTransferFrom(sender, address(this), amount);
70:         } else { // ETH
-71:             if(msg.value < amount) { revert TransferFailed(); }
+71:             if(msg.value != amount) { revert TransferFailed(); }
72:         }

Lines of code

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

Vulnerability details

_close() first transfers the token to the lender, then deletes the credit line in storage.

This breaks the CEI pattern, meaning if the credit.token has a receiver hook - such as an ERC77 token - a malicious lender can reenter the close() call to transfer themselves credit.deposit + credit.interestRepaid several times.

For this to happen, there needs to be an amount of credit.token in the LineOfCredit greater than credit.deposit + credit.interestRepaid.

This is possible if a credit line was added for the same credit.token by another lender, which would compute a different id despite the token being the same.

In summary, this attack can happen if:

• credit.token is a token with hooks
• another credit line for the same token has been created by another lender (or several).

In this case, the other lenders having created a credit line for the same token will have lost funds.

Impact

Medium

Tools Used

Manual Analysis

Mitigation

Add a reentrancy check to _close()

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L237 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L68-L69

Vulnerability details

When a lender wants to create a credit line or add credit to an existing credit line, the LineLib.receiveTokenOrETH method is used. It separates two cases: if the token is ETH or not.

In the case of an ERC-20 token, msg.value is not checked. If a lender mistakenly passes a non-zero msg.value while adding credit, this ETH will be lost, as there is no function in LineOfCredit() or LineLib to recover ETH.

Note that there is indeed a sendOutTokenOrETH method that transfers ETH out of the contract, however:

• In borrow(), the call would revert here, as that lost msg.value was not taken into account in credit.deposit
• In withdraw(), the call would revert here, again because the lost msg.value was not taken into account in credit.deposit
• In close(), there is no such checks and the call would go through. But as it is repayment, it means a borrower would only need to transfer credit.deposit + credit.interestRepaid - lostMsgValue into the contract, then call close() to close the credit line, repaying the lender. So while the lender retrieved that lostMsgValue, it was used by the borrower during repayment.

Therefore in any case, that lostMsgValue is lost to the lender.

Impact

Medium

Tools Used

Manual Analysis

Mitigation

Add a check to ensure msg.value == 0 if the token is not ETH.

67:         if(token == address(0)) { revert TransferFailed(); }
68:         if(token != Denominations.ETH) { // ERC20
+               require(msg.value == 0, "no ETH allowed for ERC20")
69:             IERC20(token).safeTransferFrom(sender, address(this), amount);
70:         } else { // ETH
71:             if(msg.value < amount) { revert TransferFailed(); }
72:         }

Lines of code

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

Vulnerability details

LineLib.sendOutTokenOrETH() - which is used in LineOfCredit to send a borrower their principal, and to send a lender their withdrawal - uses the native transfer() method to transfer ETH.

The use of the deprecated transfer() function will fail when:

• The receiver smart contract implements a payable fallback which uses more than 2300 gas unit.
• The receiver smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

This means lenders and borrowers will be limited to EOAs or smart contracts with low gas consumption when receiving ETH. This not desirable, especially given the protocol desire to have LineOfCredit used as a building block for more complex financial operations in the future.

Impact

Medium

Proof of Concept

Run the foundry test in Transfer.t.sol from this private gist.

It tries to send ETH using a function via transfer() to two wallets:

• wallet1, which is a payment splitter wallet that transfers ETH to defined beneficiaries upon receiving ETH.
• wallet2, a simple smart contract wallet with no logic.

You can see that the transfer to wallet1 fails with an out of gas error, while the transfer to wallet2 works properly.

    ├─ [12002] PoCTransfer::withdraw(Wallet: [0x5b851f32f9ab7fb2c76480c608034a5ed1f16cfc], 1000000000000000000) 
    │   ├─ [2277] Wallet::fallback{value: 1000000000000000000}() 
    │   │   └─ ← "EvmError: OutOfGas"
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Tools Used

Manual Analysis, Foundry

Mitigation

Use call() instead of transfer

-48:             payable(receiver).transfer(amount);
+48:             payable(receiver).call{value: amount}(new bytes(0));

QA Report

The protocol functionality was easy to follow thanks to a thorough documentation with detailed flow diagrams and a well commented code base

A few logic issues were found. They are minor but are worth taking a look at, given that the protocol has created a deFi primitive - the Line of Credit - and composability with external contracts is important.

L01 - Denial Of Service if credit lines number too high, making repayments impossible

https://github.com/debtdao/Line-of-Credit/blob/6987988fe39901cad9a8e5ebb2c6aa719590873d/contracts/modules/credit/LineOfCredit.sol#L472 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L498 https://github.com/debtdao/Line-of-Credit/blob/6987988fe39901cad9a8e5ebb2c6aa719590873d/contracts/utils/CreditListLib.sol#L51-L54 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L449

The current logic is that _close() checks that a credit line is fully repaid and removes it, by doing a call to removePosition.

The issue is that removePosition does not actually remove the id from the ids array, it simply set it to 0: the delete statement in solidity set an element to 0 but does not remove it from the array.

On each LineOfCredit._repay() call, a call to stepQ is made, which loops through the entire ids array to re-order it.

This means that if too many credit lines are added, the stepQ calls will always revert, essentially preventing a borrower from repaying their debts.

Each additional credit line adds roughly ~5,000 gas (1 SLOAD and 1 SSTORE) to a _repay() call. Given the 30M gas block limit, this means DoS will happen if a Line of Credit ids exceeds a certain number M between 5,000 and 6,000.

After discussion with the team, this number of credit lines was however deemed unlikely given that there can be only one credit line per token.

Impact

Low

Proof Of Concept

The borrower and the lenders agree on the first credit line. The borrower calls borrow() and receives a loan on this first credit line. The lenders and the borrowers agree to add N credit lines, and call addCredit(), pushing N credits to ids.

The borrower tries to call depositAndClose() to close the first credit line, but the call to _repay() will revert, as the stepQ call will run out of the gas before finishing to loop through all the ids of the array.

Tools Used

Manual Analysis

Mitigation

Consider setting a MAX_LINES that ids.length cannot exceed, preventing too many credit lines from being created.

L02 - _close() should not be able to close a specific id credit line

As per the docs:

Can a Borrower chose to repay any debt in any order?
No. The app automatically selects which credit line can be repaid using a first-in-first-out (FIFO) repayment queue based on the order in which credit lines are drawn down

The issue is that close() takes an id argument, meaning it technically breaks this protocol requirement if called for any id other than ids[0]. Now, because of this credit.principal check, it is actually impossible to close any id other than the first one - because it is only possible to repay the full debt through depositAndClose and depositAndRepay, which always repay the first id in the array ids.

close() can hence only work for id = ids[0] anyway.

Because of the protocol desire for it to be composable, this can open risks if a child contract inheriting LineOfCredit were to give the possibility to repay full debt of any id credit line. This would mean a borrower could repay and close in any order.

Impact

Low

Tools Used

Manual Analysis

Mitigation

Modify both close() and _close(), so that they can only close the first credit line in queue

-388:     function close(bytes32 id) external payable override returns (bool) {
+388:     function close() external payable override returns (bool) {
+            bytes32 id = ids[0];
389:         Credit memory credit = credits[id];
...
-406:         _close(credit, id);
+406:         _close(credit);

-483: function _close(Credit memory credit, bytes32 id) internal virtual returns (bool) {
+483: function _close(Credit memory credit) internal virtual returns (bool) {
+            bytes32 id = ids[0];

L03 - close underflow if called when count == 0

When deleting a position by calling close, the internal _close call decreases count - the state variable representing the open credit lines.

The issue is that the decrement is unchecked. So if a borrower calls close() when count == 0, the operation will underflow and count will be equal to type(uint256).max.

This does not leave to any direct vulnerability, as all credits will be empty and the Line status will be REPAID anyway, but this does mean the counts() function will return count == type(uint256).max, which can be misleading to any external contract using the LineOfCredit API, and even open to vulnerabilities if there is logic performed based on the reading of count.

Impact

Low

Proof of Concept

Add the following test to LineOfCredit.t.sol:

function test_can_underflow_count() public {
    _addCredit(address(supportedToken1), 1 ether);
    bytes32 id = line.ids(0);
    hoax(borrower);
    line.borrow(id, 1 ether);
    hoax(borrower);
    line.depositAndRepay(1 ether);
    (uint p, uint i) = line.updateOutstandingDebt();
    assertEq(p + i, 0, "Line outstanding credit should be 0");
    hoax(borrower);
    line.close(id);

    // @audit borrower calls `count()` again
    hoax(borrower);
    line.close(id);

    // @audit count underflowed
    (uint count, uint len)= line.counts();
    assertEq(count, type(uint256).max, "count should be max");
}

Tools Used

Manual Analysis, Foundry

Mitigation

Leave the underflow check when decrementing count.

-499:      unchecked { --count; }
+499:      --count;

You can also add the whileBorrowing modifier to close(), to revert calls when count == 0

L04 - Logic error in SpigotedLine.releaseSpigot()

ISpigotedLine describes releaseSpigot() as follow:

95:     * @notice - Transfers ownership of the entire Spigot from its then Owner to either the Borrower (if a Line of Credit has been been fully repaid) 
96:                 or to the Arbiter (if the Line of Credit is liquidatable).
97:     * @dev    - callable by borrower + arbiter

The function calls the releaseSpigot() method of SpigotedLineLib. This method is described as:

191:    * @dev    - callable by anyone 

This is the first problem: a mismatch in definitions between ISpigotedLine saying it should only be call by the arbiter or borrower, and the library stating there is no access control.

The second issue is that while both descriptions state the ownership should be transferred to either the borrower (if REPAID) or the arbiter (if liquidatable), the function actually transfers it to a to parameter that can be any address.

Impact

Low

Tools Used

Manual Analysis

Mitigation

The following mitigation ensures the spigot ownership can only be transferred to either the arbiter or the borrower. You should also update ISpigotedLine and SpigotedLineLib so that the access control of the function is the same in both descriptions.

194:     function releaseSpigot(address spigot, LineLib.STATUS status, address borrower, address arbiter, address to) external returns (bool) {
195:         if (status == LineLib.STATUS.REPAID &&  msg.sender == borrower) {
-196:           if(!ISpigot(spigot).updateOwner(to)) { revert ReleaseSpigotFailed(); }
+196:           if(!ISpigot(spigot).updateOwner(borrower)) { revert ReleaseSpigotFailed(); }
197:           return true;
198:         }
199: 
200:         if (status == LineLib.STATUS.LIQUIDATABLE && msg.sender == arbiter) {
-201:           if(!ISpigot(spigot).updateOwner(to)) { revert ReleaseSpigotFailed(); }
+201:           if(!ISpigot(spigot).updateOwner(arbiter)) { revert ReleaseSpigotFailed(); }
202:           return true;
203:         }

L05 - Logic error in SpigotedLine.sweep()

ISpigotedLine describes sweep() as follow:

104:    * @notice - sends unused tokens to borrower if REPAID or arbiter if LIQUIDATABLE or INSOLVENT

106:    * @dev    - callable by anyone 

But the function calls the sweep() method of SpigotedLineLib with a to parameter that can be any address, not just the borrower or the arbiter.

The function also only transfers the amount to to if the caller is the borrower and the line is repaid, or if the caller is the arbiter and the status is LIQUIDATABLE or INSOLVENT. Any other msg.sender would result in the call reverting.

Impact

Low

Tools Used

Manual Analysis

Mitigation

Update SpigotedLineLib.sweep() to suit the description in ISpigotedLine:

the function should:

• be callable by anyone.
• transfers amount to borrower if the status is REPAID.
• transfers amount to arbiter if the status is LIQUIDATABLE or INSOLVENT.

217:     function sweep(address to, address token, uint256 amount, LineLib.STATUS status, address borrower, address arbiter) external returns (bool) {
218:         if(amount == 0) { revert UsedExcessTokens(token, 0); }
219: 
-220:         if (status == LineLib.STATUS.REPAID && msg.sender == borrower) {
-221:             return LineLib.sendOutTokenOrETH(token, to, amount);
-222: 
+220:         if (status == LineLib.STATUS.REPAID) {
+221:             return LineLib.sendOutTokenOrETH(token, borrower, amount);
223:         }
224: 
-225:         if (
-226:           (status == LineLib.STATUS.LIQUIDATABLE || status == LineLib.STATUS.INSOLVENT) &&
-227:           msg.sender == arbiter
-228:         ) {
-229:             return LineLib.sendOutTokenOrETH(token, to, amount);
+225:         if (
+226:           (status == LineLib.STATUS.LIQUIDATABLE || status == LineLib.STATUS.INSOLVENT)){
+229:             return LineLib.sendOutTokenOrETH(token, arbiter, amount);
230:         }
231: 
-232:         revert CallerAccessDenied();

L06 - Last statement in SpigotedLineLib.sweep() is never reached

The sweep method of the SpigotedLineLib library is used to sweep remaining tokens in the Spigot.

If the conditions line 220 and 226 are not met, the call reverts.

There is however a return false statement line 234, after the revert statement.

This return statement will never be reached, as the call will revert beforehand every time.

Impact

Low

Tools Used

Manual Analysis

Mitigation

Either have the function revert and remove return false, or remove the revert() statement.

NC01 - typos

• demoninated -> denominated