Debt DAO contest - Lambda's results

Lines of code

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

Vulnerability details

Impact

Neither SpigotLib.claimRevenue nor SpigotLib._claimRevenue check that the provided revenueContract was registered before. If this is not the case, SpigotLib._claimRevenue assumes that this is a revenue contract with push payments (because self.settings[revenueContract].claimFunction is 0) and just returns the difference since the last call to claimRevenue:

       if(self.settings[revenueContract].claimFunction == bytes4(0)) {
            // push payments

            // claimed = total balance - already accounted for balance
            claimed = existingBalance - self.escrowed[token]; //@audit Rebasing tokens
            // underflow revert ensures we have more tokens than we started with and actually claimed revenue
        }

SpigotLib.claimRevenue will then read self.settings[revenueContract].ownerSplit, which is 0 for non-registered revenue contracts:

uint256 escrowedAmount = claimed * self.settings[revenueContract].ownerSplit / 100;

Therefore, the whole claimed amount is sent to the treasury.

This becomes very problematic for revenue tokens that use push payments. An attacker (in practice the borrower) can just regularly call claimRevenue with this token and a non-existing revenue contract. All of the tokens that were sent to the spigot since the last call will be sent to the treasury and none to the escrow, i.e. a borrower can ensure that no revenue will be available for the lender, no matter what the configured split is.

Proof Of Concept

As mentioned above, the attack pattern works for arbitrary tokens where one (or more) revenue contracts use push payments, i.e. where the balance of the Spigot increases from time to time. Then, the attacker just calls claimRevenue with a non-existing address. This is illustrated in the following diff:

--- a/contracts/tests/Spigot.t.sol
+++ b/contracts/tests/Spigot.t.sol
@@ -174,7 +174,7 @@ contract SpigotTest is Test {
         assertEq(token.balanceOf(address(spigot)), totalRevenue);
         
         bytes memory claimData;
-        spigot.claimRevenue(revenueContract, address(token), claimData);
+        spigot.claimRevenue(address(0), address(token), claimData);

Thanks to this small modification, all of the tokens are sent to the treasury and none are sent to the escrow.

Recommended Mitigation Steps

Check that a revenue contract was registered before, revert if it does not.

Lines of code

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

Vulnerability details

Impact

The functions addCredit and increaseCredit both ahve a mutualConsent or mutualConsentById modifier. Furthermore, these functions are payable and the lender needs to send the corresponding ETH with each call. However, if we look at the mutual consent modifier works, we can a problem:

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

The problem is: On the first call, when the other party has not given consent to the call yet, the modifier does not revert. It sets the consent of the calling party instead.

This is very problematic in combination with sending ETH for two reasons: 1.) When the lender performs the calls first and sends ETH along with the call, the call will not revert. It will instead set the consent for him, but the sent ETH is lost. 2.) Even when the lender thinks about this and does not provide any ETH on the first call, the borrower has to perform the second call. Of course, he will not provide the ETH with this call, but this will cause the transaction to revert. There is now no way for the borrower to also grant consent, but still let the lender perform the call.

Proof Of Concept

Lender Alice calls LineOfCredit.addCredit first to add a credit with 1 ETH. She sends 1 ETH with the call. However, because borrower Bob has not performed this call yet, the function body is not executed, but the 1 ETH is still sent. Afterwards, Bob wants to give his consent, so he performs the same call. However, this call reverts, because Bob does not send any ETH with it.

Recommended Mitigation Steps

Consider implementing an external function to grant consent to avoid this scenario. Also consider reverting when ETH is sent along, but the other party has not given their consent yet.

Lines of code

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

Vulnerability details

Impact

In LineLib.receiveTokenOrETH, it is only checked if msg.value is greater or equal than amount when the token is ETH:

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

Therefore, when a user accidentally pays too much ETH, the additional amount will be lost. Note that this is different to ERC20 tokens, where the exact amount is enforced (by transferring the exact amount).

Proof Of Concept

Alice calls LineOfCredit.addCredit to add a credit with 1 ETH. While amount is set to 1 ETH (10**18), she accidentally sends 2 ETH with the call. The additional ETH is lost and cannot be recovered.

Recommended Mitigation Steps

Either force that the amount matches exactly or reimburse the additional ETH.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/modules/credit/SpigotedLine.sol#L165

Vulnerability details

Impact

0x supports the filling of specific orders. Furthermore, you can post orders that can only get filled by a provided address. Moreover, the only requirement in SpigotedLineLib.claimAndTrade is that the 0x call resulted in some more tokens (even if it is only 1 wei):

// underflow revert ensures we have more tokens than we started with
uint256 tokensBought = LineLib.getBalance(targetToken) - oldTargetTokens;

if(tokensBought == 0) { revert TradeFailed(); } // ensure tokens bought

Furthermore, a borrower can call arbitrary 0x functions by providing the corresponding zeroExTradeData (which is passed as calldata to 0x).

A malicious borrower can exploit this in the following way: 1.) Post a 0x order to buy the whole revenue amount that has accrued in the Spigot contract for an extremely low amount of the target token (e.g., 1 cent). 2.) Call SpigotedLine.claimAndTrade to fill this order (by providing the corresponding call data) and get the revenue tokens for free.

An attacker can use this attack pattern continously to siphon all revenue tokens out of the spigot. Like that, he never has to pay back the spigoted line of credit.

Proof Of Concept

Let's say there is a spigoted line of credit with some revenue token TOK with a value of 1,000 USD. The borrower has taken out a loan in USDC, so the token for the credit is USDC. Since the last spigot.claimEscrow call, 100 TOK were accrued (with a value of 100,000 USD). The attacker uses the attack pattern that is described above to sell all 100 TOK (to himself via an order that he posted earlier) for 1 USDC. This leads to a loss of 100,000 USD for the lender.

Recommended Mitigation Steps

Consider implementing an external function to grant consent to avoid this scenario. Also consider reverting when ETH is sent along, but the other party has not given their consent yet.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/d7ef66035ddf873b0c96804a1c9deeebb1f798ea/contracts/utils/LineLib.sol#L69 https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotLib.sol#L38

Vulnerability details

Impact & Proof Of Concept

The function LineLib.receiveTokenOrETH that is used in multiple places assumes that the requested transfer amount is also equal to the actual transferred amount:

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

This is not true for fee-on-transfer tokens, where the change in balance is smaller than the requested amount. This leads to problems in multiple places. For instance, in LineOfCredit.addCredit, the amount parameter is used to create the credit:

LineLib.receiveTokenOrETH(token, lender, amount);

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

When the contract does not contain this amount, there are two options: 1.) Payouts that should be possible are not possible (because the balance is not sufficient). 2.) Funds from other users are used for payouts.

Another big problem for the system are rebasing tokens. This can cause an underflow in SpigotLib._claimRevenue (because the balance is now smaller than the previously cached balance), making all calls to claim revenue revert:

uint256 existingBalance = LineLib.getBalance(token);
...

claimed = existingBalance - self.escrowed[token];

Recommended Mitigation Steps

Check the actual amount that was transferred (i.e., the difference of the balances).