Debt DAO contest - cccz's results

Lines of code

https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L59-L74

Vulnerability details

Impact

In LineLib.receiveTokenOrETH, msg.value >= amount is required, which means if msg.value > amount, excess ETH will not be refunded.

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

Proof of Concept

https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L59-L74

Tools Used

None

Recommended Mitigation Steps

Refund the excess ETH.

Lines of code

https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/SpigotedLineLib.sol#L120-L140

Vulnerability details

Impact

The lender can call the claimAndRepay function, which will call SpigotedLineLib.trade. In SpigotedLineLib.trade, when claimToken == ETH, ETH will be sent to swapTarget. If the zeroExTradeData.inputTokenAmount provided by the lender is less than amount ,the excess ETH may remain in the swapTarget contract.

   function trade(
        uint256 amount,
        address sellToken,
        address payable swapTarget,
        bytes calldata zeroExTradeData
    ) 
        public
        returns(bool)
    {
        if (sellToken == Denominations.ETH) {
            // if claiming/trading eth send as msg.value to dex
            (bool success, ) = swapTarget.call{value: amount}(zeroExTradeData);
            if(!success) { revert TradeFailed(); }
        } else {
            IERC20(sellToken).approve(swapTarget, amount);
            (bool success, ) = swapTarget.call(zeroExTradeData);
            if(!success) { revert TradeFailed(); }
        }

        return true;
    }

Proof of Concept

https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/SpigotedLine.sol#L93-L115 https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/SpigotedLineLib.sol#L120-L140 https://github.com/0xProject/protocol/blob/development/contracts/zero-ex/contracts/src/features/TransformERC20Feature.sol#L254-L263

Tools Used

None

Recommended Mitigation Steps

Since the borrower has sufficient incentive to call claimAndRepay to pay off the debt and reduce the interest expense, consider disabling the lender from calling the claimAndRepay function.

Lines of code

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

Vulnerability details

Impact

There are ERC20 tokens that charge fee for every transfer() or transferFrom(). When using fee-on-transfer tokens in contracts, there are some unexpected situations. For example, in the depositAndClose function, the number of tokens received by the contract will be less than totalOwed, and then sending tokens to the lender in _close will fail due to insufficient balance.

Proof of Concept

https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L292-L311 https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L483-L493

Tools Used

None

Recommended Mitigation Steps

Consider getting the received amount by calculating the difference of token balance (using balanceOf) before and after the transferFrom.

Lines of code

https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L34-L51

Vulnerability details

Impact

In LineLib, sendOutTokenOrETH function calls native payable.transfer. This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract.

Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

Proof of Concept

https://github.com/debtdao/Line-of-Credit//blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L34-L51

Tools Used

None

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60