Debt DAO contest - HE1M's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L234 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/LineLib.sol#L71

Vulnerability details

Impact

If a lender would like to lend ERC20 token, the protocol works properly. The lender gives allowance to the line of credit and calls addCredit to sign the consent. Later, the borrower calls this function, and signs the consent. Since two signatures are provided, the credit will be created and the ERC20 token will be transferred from lender to the line of credit contract. Everything is working as expected.

But if the lender would like to lend ETH, and call addCredit earlier than borrower, the lender will lose the fund completely. Please read the proof of concept section regarding this scenario.

Proof of Concept

Based on the document:

To create a credit line within the Line of Credit facility, a Lender needs to make a first deposit. The deposit can only succeed if both Borrower and Lender consent and mutually sign a 2/2 transaction as evidence of agreement.

Suppose a lender decides to make a first ETH deposit. So, s/he calls addCredit and transfers some amount of ETH. Since, the borrower has not yet called this function, the lender's call only transfers the ETH and sign the consent in the mapping mutualConsents[newHash] = true. But the credit will not be created, because the modifier mutualConsent needs two signatures to proceed with credit creation.

function addCredit(
        uint128 drate,
        uint128 frate,
        uint256 amount,
        address token,
        address lender
    )
        external
        payable
        override
        whileActive
        mutualConsent(lender, borrower)
        returns (bytes32)
    {
        LineLib.receiveTokenOrETH(token, lender, amount);

        bytes32 id = _createCredit(lender, token, amount);

        require(interestRate.setRate(id, drate, frate));
        
        return id;
    }

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

modifier mutualConsent(address _signerOne, address _signerTwo) {
      if(_mutualConsent(_signerOne, _signerTwo))  {
        // Run whatever code needed 2/2 consent
        _;
      }
    }

function _mutualConsent(address _signerOne, address _signerTwo) internal returns(bool) {
        if(msg.sender != _signerOne && msg.sender != _signerTwo) { revert Unauthorized(); }

        address nonCaller = _getNonCaller(_signerOne, _signerTwo);

        // The consent hash is defined by the hash of the transaction call data and sender of msg,
        // which uniquely identifies the function, arguments, and sender.
        bytes32 expectedHash = keccak256(abi.encodePacked(msg.data, nonCaller));

        if (!mutualConsents[expectedHash]) {
            bytes32 newHash = keccak256(abi.encodePacked(msg.data, msg.sender));

            mutualConsents[newHash] = true;

            emit MutualConsentRegistered(newHash);

            return false;
        }

        delete mutualConsents[expectedHash];

        return true;
    }

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L50

So, the ETH is withdrawn from lender and received by LineOfCredit, but no credit is created yet. Later when borrower calls addCredit, it will be reverted, because msg.value is not equal to amount.

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

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

The situation can be even worse, if the borrower tricks the lenders and tell them that s/he (borrower) has already called addCredit, so the lenders will assume that it is safe to transfer ETH trough calling addCredit(...), but in reality the borrower has not called it yet. So, the lenders call this function, and will lost their ETH. They can not even withdraw it, because no credit is created for lenders.

Tools Used

Recommended Mitigation Steps

There can be some solutions:

• If the lender is going to send ETH, s/he should be after the borrower.
• The ETH sent by lender should be tracked in the modifier by a mapping pendingDeposit[lender] = msg.value. So that if borrower later calls this function, it checks this mapping, and proceeds to the next step. It should be also claimable by the lender.
• If lender is sending ETH, and the modifier returns false, then it should revert.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L38

Vulnerability details

Impact

There is no mechanism to cancel a consent. This can happen if a lender decides to not consent on an agreement OR the parameters sent in function addCredit are not correct.

Proof of Concept

If for example a lender consented already by invoking the function addCredit, then there is no mechanism to cancel this consent. Because, any function with theses modifiers mutualConsent and mutualConsentById will set the consent in the mapping mutualConsents, and there is no way to delete it except if other party (borrower) executes this consent (that is a pity for the lender who wanted to cancel).

function _mutualConsent(address _signerOne, address _signerTwo) internal returns(bool) {
        if(msg.sender != _signerOne && msg.sender != _signerTwo) { revert Unauthorized(); }

        address nonCaller = _getNonCaller(_signerOne, _signerTwo);

        // The consent hash is defined by the hash of the transaction call data and sender of msg,
        // which uniquely identifies the function, arguments, and sender.
        bytes32 expectedHash = keccak256(abi.encodePacked(msg.data, nonCaller));

        if (!mutualConsents[expectedHash]) {
            bytes32 newHash = keccak256(abi.encodePacked(msg.data, msg.sender));

            mutualConsents[newHash] = true;

            emit MutualConsentRegistered(newHash);

            return false;
        }

        delete mutualConsents[expectedHash];

        return true;
    }

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L38 https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L223

This applies to all functions that contain these modifiers.

Tools Used

Recommended Mitigation Steps

A simple function just to delete a consent should be added.

Judge has assessed an item in Issue #35 as M risk. The relevant finding follows:

No. 2 Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300. If gas costs are subject to change, then smart contracts can’t depend on any particular gas costs.

function sendOutTokenOrETH( address token, address receiver, uint256 amount ) external returns (bool) { if(token == address(0)) { revert TransferFailed(); }

    // both branches revert if call failed
    if(token!= Denominations.ETH) { // ERC20
        IERC20(token).safeTransfer(receiver, amount);
    } else { // ETH
        payable(receiver).transfer(amount);
    }
    return true;
}

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

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L135

Vulnerability details

Impact

During trading claimToken with targetToken, the calldata provided for the 0x router is an arbitrary data that can be provided by the borrower or lender. https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L135

If lender provides the calldata so that that more claimToken is consumed than expected, the borrower is impacted (loses some fund).

Proof of Concept

If a lender (malicious) would like to claim and repay, the flow is as follows: claimAndRepay(address claimToken, bytes calldata zeroExTradeData) => function _claimAndTrade(address claimToken, address targetToken, bytes calldata zeroExTradeData) => function claimAndTrade(address claimToken, address targetToken, address payable swapTarget, address spigot, uint256 unused, bytes calldata zeroExTradeData) => function trade(uint256 amount, address sellToken, address payable swapTarget, bytes calldata zeroExTradeData)

https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/modules/credit/SpigotedLine.sol#L93 https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/modules/credit/SpigotedLine.sol#L187 https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L53 https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L120

Assuming the targetToken is DAI, and claimToken is USDC, if the calldata zeroExTradeData is something that calls the function buy 100 DAI for any amount of USDC instead of buy 100 DAI for maximum amount of 105 USDC, it is highly possible that more than 105 USDC will be consumed to trade for 100 DAI. In this case, the borrower loses more claimToken than expected.

In summary, since the calldata is arbitrary, the lender can put these data buy X targetToken for any amount of claim token or sell X claimToken for any amount of targetToken, in both cases the borrower may lose fund. While the correct call data is: buy X targetToken for max Y claimToken or sell X claimToken for min Y targetToken.

Tools Used

Recommended Mitigation Steps

The calldata zeroExTradeData should be set in advance that which function of the router is going to be called. For example in case of uniswap router, it should be abi.encodeWithSignature("swapExactTokensForTokens(uint256,uint256,address[],address,uint256)", amountIn, amountOutMin, path, to, dedline). In which the amountIn and amountOutMin should be a factor of each other so the borrower will not lose revenue token a lot.

No. 1

Better to check that if an ERC20 token is going to be transferred to the protocol, no ETH should be sent:

function receiveTokenOrETH(
      address token,
      address sender,
      uint256 amount
    )
      external
      returns (bool)
    {
        if(token == address(0)) { revert TransferFailed(); }
        if(token != Denominations.ETH) { // ERC20
            // this line should be added:
            require(msg.value == 0);
            IERC20(token).safeTransferFrom(sender, address(this), amount);
        } else { // ETH
            if(msg.value < amount) { revert TransferFailed(); }
        }
        return true;
    }

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

No. 2

Any smart contract that uses transfer() or send() is taking a hard dependency on gas costs by forwarding a fixed amount of gas: 2300. If gas costs are subject to change, then smart contracts can’t depend on any particular gas costs.

function sendOutTokenOrETH(
      address token,
      address receiver,
      uint256 amount
    )
      external
      returns (bool)
    {
        if(token == address(0)) { revert TransferFailed(); }
        
        // both branches revert if call failed
        if(token!= Denominations.ETH) { // ERC20
            IERC20(token).safeTransfer(receiver, amount);
        } else { // ETH
            payable(receiver).transfer(amount);
        }
        return true;
    }

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

No. 3

It is possible to apply grieving attack. When a borrower is going to call rollover(address newLine), the attacker calls the function init() in the address of new line, so the status of the new line will change to LineLib.STATUS.ACTIVE. https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/modules/credit/SecuredLine.sol#L48

Now, the borrower transaction for rolling over will be reverted, because the status of new line is not LineLib.STATUS.UNINITIALIZED. https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/modules/credit/SecuredLine.sol#L57

No. 4

Line 234 will never be called, it can be removed: https://github.com/debtdao/Line-of-Credit/blob/c1037d7f873cfb2af48fdaf00e44cd55c7737fc1/contracts/utils/SpigotedLineLib.sol#L234

No. 5

Maybe better to use staticcall to get decimals: https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/CreditLib.sol#L142