Debt DAO contest - cloudjunky's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/LineLib.sol#L68

Vulnerability details

Impact

receiveTokenOrETH() is used throughout LineOfCredit.sol (e.g. addCredit(), increaseCredit()) and Escrow.sol (via EscrowLib.sol) to receive funds in the form of ETH or ERC20 tokens.

The receiveTokenOrETH() requires the caller to send ETH with the token address set to 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE OR send ERC20 tokens with the token address set to an approved token (decided by the arbiter).

The issue is that the caller can send ETH with the token not set to 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE resulting in both ETH and ERC20 being deposited at the same time.

This causes issues with accounting across the project and the loss of ETH if it is sent alongside a supported ERC20 token.

Proof of Concept

The foundry test below added to Escrow.t.sol demonstrates that you can send ETH alongside an ERC20 token. The ETH is accepted but not accounted for as only the supported ERC20 token address (that is not all E’s) is added as collateral.

function test_can_send_eth_and_token() public {
  uint borrowerBalance = borrower.balance;
  uint borrowerTokenBalance = supportedToken1.balanceOf(borrower);
  uint escrowBalance = address(escrow).balance;
  escrow.addCollateral{value: mintAmount}(mintAmount, address(supportedToken1));
  assertEq(
    borrowerBalance,
    borrower.balance + mintAmount,
    "borrower should have decreased with collateral deposit"
  );
  assertEq(
    borrowerTokenBalance,
    supportedToken1.balanceOf(borrower) + mintAmount,
    "borrower should have decreased token collateral"
  );
  assertEq(
    escrowBalance,
    address(escrow).balance - mintAmount,
    "escrow should have increased with collateral deposit"
  );
}

Tools Used

Vim

Recommended Remediation Steps

receiveTokenOrETH() should revert if it receives any ETH when the ERC20 token address it not 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE. For example (the + line is added);

function receiveTokenOrETH(
  address token,
  address sender,
  uint256 amount
)
  external
  returns (bool)
{
    if(token == address(0)) { revert TransferFailed(); }
    if(token != Denominations.ETH) { // ERC20
+       require(msg.value <= 0, "Don't send ETH on ERC20 transfers");
        IERC20(token).safeTransferFrom(sender, address(this), amount);
    } else { // ETH
        if(msg.value < amount) { revert TransferFailed(); }
    }
    return true;
}

Lines of code

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

Vulnerability details

Impact

The transfer() function only provides a stipend for the recipient to use of 2300 gas. If the recipient uses more than that, transfers will fail. For longevity it is better to use call() rather than transfer() to ensure that future gas increases won’t affect ETH transfers.

The transfer() function can fail for a number of reasons;

1. The claimer smart contract does not implement a payable function.
2. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The claimer 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.

Additionally, using higher than 2300 gas might be mandatory for some multi-sig wallets.

Recommended mitigation steps

The following git diff demonstrates how call() can be used instead of transfer();

diff --git a/contracts/utils/LineLib.sol b/contracts/utils/LineLib.sol
index fb112cb..97ddbf2 100644
--- a/contracts/utils/LineLib.sol
+++ b/contracts/utils/LineLib.sol
@@ -45,7 +45,8 @@ library LineLib {
     if(token!= Denominations.ETH) { // ERC20
         IERC20(token).safeTransfer(receiver, amount);
     } else { // ETH
-            payable(receiver).transfer(amount);
+            (bool success,) = payable(receiver).call{value: amount}("");
+            require(success, "ETH transfer failed");
     }
     return true;
 }

Note call() introduces the potential for re-entrancy but future proofs ETH transfers into the future in the even that gas usage changes. Functions including call should follow the Check, Effects, Interactions pattern and or Openzeppelin’s ReentrancyGuard.