Funds can be added and not accrue as collateral

[code423n4]

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

[c4-judge]

dmvt marked the issue as primary issue

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-sponsor]

kibagateaux marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as satisfactory

[C4-Staff]

liveactionllama marked the issue as duplicate of #355