Debt DAO contest - Tomo's results

Lines of code

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

Vulnerability details

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

/**
 * @notice - Receive ETH or ERC20 token at this contract from an external contract
 * @param token - address of token to receive. Denominations.ETH for raw ETH
 * @param sender - address that is sendingtokens/ETH
 * @param amount - amount of tokens to send
 */
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;
}

1. Alice executes the addCredit() function. Alice was going to pay 1 ETH as an initial credit.
2. However, Alice mistakenly set msg.value to 10 ETH.
3. After executing this function, Alice noticed too much paid but it doesn’t refund because no implementation to measure too much ETH in this contract.

Impact

User may lose their ETH to overpay ETH

Proof of Concept

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

Tools Used

Recommended Mitigation Steps

変更してね

You should choose one of these options

1. Ensure the fee equal to msg.value before transferring ERC20 tokens or NFT.

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

2. If msg.value is too much paid, refund to msg.sender

if(token != Denominations.ETH) { // ERC20
        IERC20(token).safeTransferFrom(sender, address(this), amount);
    } else { // ETH
        if(msg.value < amount) { revert TransferFailed(); } 
              uint gap = amount - msg.value;
	     if(gap > 0) {
		  (bool success, bytes memory data) = msg.sender.call{value: gap }("");
              }
	     require(success, "Failed to send Ether");		
    }

Lines of code

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

Vulnerability details

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

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

Users may mistakenly execute this function with non-zero msg.value white setting the token set non Denominations.ETH

The ETH would be passed to the contract and the user would lose that ETH.

Impact

Possible lost msg.value due to insufficient checking

Proof of Concept

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

Tools Used

Manual

Recommended Mitigation Steps

Consider reverting the transaction when the msg.value is not 0

function receiveTokenOrETH(
      address token,
      address sender,
      uint256 amount
    )
      external
      returns (bool)
    {
				if(msg.value != 0) {revert(); }
        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;
    }

Lines of code

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

Vulnerability details

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

function sendOutTokenOrETH(
      address token,
      address receiver,
      uint256 amount
    )
      external
      returns (bool)
    {
 				/* ... */
        if(token!= Denominations.ETH) { // ERC20
            IERC20(token).safeTransfer(receiver, amount);
  				/* ... */
        return true;
    }

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

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 multisig wallets.

Proof of Concept

You can see more detail about this issue

https://solidity-by-example.org/sending-ether/

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual

Recommended Mitigation Steps

Use call() instead of transfer() when transferring ETH