Changing auction duration will have effect on ongoing auctions

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-07-benddao/blob/main/src/libraries/logic/ConfigureLogic.sol#L504 https://github.com/code-423n4/2024-07-benddao/blob/117ef61967d4b318fc65170061c9577e674fffa1/src/libraries/logic/IsolateLogic.sol#L320-L321 https://github.com/code-423n4/2024-07-benddao/blob/117ef61967d4b318fc65170061c9577e674fffa1/src/libraries/logic/IsolateLogic.sol#L423-L424

Vulnerability details

Impact

Auctions created before the auction duration was changed will have their duration shortened/extended, which may catch their participants off guard.

Proof of Concept

When an isolated position reaches it's liquidation threshold, users can put it up for auction to pay off the debt and take the collateral NFT. The auction has a fixed duration during which borrower can redeem the debt and if it doesn't happen, the last bidder will pay off the debt and receive the NFT at the end. The pool admin can customize the auctionDuration variable with the setAssetAuctionParams function. This can be a problem because the protocol checks whether the auction has ended as follows:

      vars.auctionEndTimestamp = loanData.bidStartTimestamp + nftAssetData.auctionDuration;
      require(block.timestamp > vars.auctionEndTimestamp, Errors.ISOLATE_BID_AUCTION_DURATION_NOT_END);

      vars.auctionEndTimestamp = loanData.bidStartTimestamp + nftAssetData.auctionDuration;
      require(block.timestamp <= vars.auctionEndTimestamp, Errors.ISOLATE_BID_AUCTION_DURATION_HAS_END);

Thus, the new duration will be applied to auctions that were started before the duration change. For example, if the duration was shortened, some auctions will end prematurely, which may be unexpected for its participants.

Tools Used

Manual review

Recommended Mitigation Steps

Consider calculating and storing the auction end time instead of the start time:

-       loanData.bidStartTimestamp = uint40(block.timestamp);
+       loanData.auctionEndTimestamp = uint40(block.timestamp) + nftAssetData.auctionDuration;

Assessed type

Timing

[c4-judge]

MarioPoneder marked the issue as primary issue

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[thorseldon]

This's our service design as expected. We will try to avoid the adjust the parameters when some auctions going on. But Maybe we want to adjust the auction duration sometimes when there's auction triggered by some emergency cases, no one can predict the emergency cases in the future, but we keep this flexibility.

[MarioPoneder]

Similar to my reasoning in #26, I understand that the centralized aspect is intended, carefully considered and out of scope. However, on contract level these parameter changes can have implications that harm users, therefore Medium severity seems justified.

Nevertheless, I am open for further discussion (also with Wardens) whether to consistently downgrade all of those DAO parameter change related issues or keep them.