Debt DAO contest - Koolex's results

Lines of code

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

Vulnerability details

Impact

The 'addCollateral()' In Escrow.sol accepts ETH as a collateral. In case the borrower passs 5 ether as an amount but mistakenly passes 6 ether as a message value, then the transaction succeeds. However, when the borrower tries to release the collateral via 'releaseCollateral()', he/she will get only the amount stored (5 ether) and not the actual deposited ETH (6 ether) which results in 1 ether that will be lost forever in the Escrow contract.

Note: this could happen also when the collateral is a token and the borrower mistakenly sent ETH with the token.

Proof of Concept

The 'addCollateral()' In Escrow.sol calls the 'addCollateral()' in EscrowLib.sol which calls 'receiveTokenOrETH()' in LineLib.sol. The amount is passed as a parameter over all those calls. In case the collateral is ETH, 'receiveTokenOrETH()' checks, if the ETH is lower than the amount passed then revert. However, there is no logic handling the case when ETH value is bigger than the amount which results in additional ETH deposited in the escrow but the stored deposit is only the amount . So when the borrower tries to release the collateral, they will get only the amount stored and not the actual deposited ETH.

A PoC in the test: Please add this function to Escrow.t.sol.

    function test_can_send_extra_ETH_as_collateral() public {
        uint borrowerBalance = borrower.balance;
        uint escrowBalance = address(escrow).balance;
        uint collateral = 10 ether; // collateral is 10 ETH

        // amount is 10 ETH
        // msg.value is 11 ETH (as the user sent mistakenly additional 1 ETH)
        escrow.addCollateral{value: collateral + 1 ether}(collateral, Denominations.ETH);
        
        uint borrowerBalance2 = borrower.balance;

        console.log('borrowerBalance before: %d',borrowerBalance);
        console.log('borrowerBalance after: %d',borrowerBalance2);

        assertEq(
                borrowerBalance,
                borrowerBalance2 + collateral ,
                "borrower should have decreased with actual collateral deposit"
        ); // This will fail

        //  assertEq(
        //         borrowerBalance,
        //         borrowerBalance2 + collateral + 1 ether,
        //         "borrower should have decreased with actual collateral deposit"
        // ); // remove the comment here so you can see this passes

    }

Then run the forge test command as follows:

forge test --match-path contracts/tests/Escrow.t.sol -vv

On the console, you should get the following result:

Logs:
  borrowerBalance before: 115792089237316195423570985008687907853269984665640564039457584007913129639935
  borrowerBalance after: 115792089237316195423570985008687907853269984665640564039446584007913129639935
  Error: borrower should have decreased with actual collateral deposit
  Error: a == b not satisfied [uint]
    Expected: 115792089237316195423570985008687907853269984665640564039456584007913129639935
      Actual: 115792089237316195423570985008687907853269984665640564039457584007913129639935

Note: in order to enable console.log you should add

import "forge-std/Test.sol";

on the top of the file.

Recommended Mitigation Steps

When ETH is the collateral

Please apply one of the following logics In 'receiveTokenOrETH()'' in LineLib.sol : 1- Check if the amount doesn't match the msg.value then revert (recommended).

Instead of

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

Replace it with

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

2- Send back the extra ETH to the borrower.

else { // ETH
  if(msg.value < amount) { revert TransferFailed();}
	if(msg.value > amount) { 
	  payable(sender).transfer(msg.value - amount);
	}
}

When non-ETH is the collateral

Instead of

if(token != Denominations.ETH) { // ERC20
IERC20(token).safeTransferFrom(sender, address(this), amount);
}

Replace it with

if(token != Denominations.ETH) { // ERC20
if(msg.value > 0) { revert TransferFailed();}
IERC20(token).safeTransferFrom(sender, address(this), amount);
}