Callers can prevent ``setAuctionHouse` happening

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L829

Vulnerability details

Impact

The current implementation of the setAuctionHouse function in LendingTerm introduces a potential limitation. The function requires that there are no auctions in progress in the current auction house, making it challenging to execute this function in lending terms with active users, especially those with short or nonexistent partial repayment intervals. Additionally, a caller could attempt to front-run the call to setAuctionHouse by initiating another position when there are no ongoing auctions.

Proof of Concept

The condition checking for no auctions in progress may be difficult to satisfy in certain scenarios:

// This condition MIGHT never be met
require(
    AuctionHouse(refs.auctionHouse).nAuctionsInProgress() == 0,
    "LendingTerm: auctions in progress"
);

Tools Used

Manual Review

Recommended Mitigation Steps

To address the limitations and potential front-running issues associated with the setAuctionHouse function, it is recommended to allow the setting of a new auction house and find an alternative method to settle the old auctions. By decoupling the auction house change from the requirement of no ongoing auctions, the function becomes more flexible and resistant to potential issues related to active user positions or front-running attempts.

Consider modifying the function to allow the setting of a new auction house without the constraint of having no auctions in progress:

// Updated setAuctionHouse function
function setAuctionHouse(address newAuctionHouse) external onlyGovernor {
    // Additional logic to settle old auctions if needed

    // Set the new auction house
    refs.auctionHouse = newAuctionHouse;
}

By implementing this modification, the function becomes more versatile, allowing the transition to a new auction house without being hindered by the presence of ongoing auctions in the current auction house. This adjustment provides greater flexibility in managing the auction house configuration.

Assessed type

Timing

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as remove high or low quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

After a proposal to setAuctionHouse is queued in the DAO timelock & ready to execute, it will revert if there are ongoing auctions. To execute & set the new auctionHouse reference, one would only need to wait for auctions to end and execute the timelock action again. There should be plenty of opportunities to update the auctionHouse reference because auctions are active only for the few minutes where an auction is ongoing after a missed periodic payment or after a term offboarding, if the term has active loans but they are not in the process of liquidation, the reference can be updated.

[c4-sponsor]

eswak (sponsor) disputed

[Trumpero]

Agree with the sponsor; this issue should be deemed invalid. To create a new auction, there must be a delay in loan repayment over time. However, the auction duration is much shorter than max delay time between partial repayments.

[c4-judge]

Trumpero marked the issue as unsatisfactory: Invalid