code-423n4 / 2024-08-wildcat-findings

3 stars 1 forks source link

Lack of validation of the `FixedTermLoanHooks.setFixedTermEndTime` function can lead to unintended behaviour #33

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L188-L192 https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L238-L240

Vulnerability details

Proof of Concept

The FixedTermLoanHooks.setFixedTermEndTime function is used to update the fixedTermEndTime of the HookedMarket. The input validation of the newFixedTermEndTime value before the update is as follows:

    if (!hookedMarket.isHooked) revert NotHookedMarket();
    if (newFixedTermEndTime > hookedMarket.fixedTermEndTime) revert IncreaseFixedTerm();
    hookedMarket.fixedTermEndTime = newFixedTermEndTime;

As it is evident from the above code snippet there is no check to ensure that newFixedTermEndTime > block.timestamp. But this is a neccesary check to ensure that the meaningful fixedTerm is set in the HookedMarket and that withdrawals can not be immediately queued by the lender before the end of the fixedTermEndTime. This is evident in the logic how the fixedTermEndTime is validated in the FixedTermLoanHooks._onCreateMarket function as shown below:

    if (
      fixedTermEndTime < block.timestamp || (fixedTermEndTime - block.timestamp) > MaximumLoanTerm
    ) { //@audit-info - MaximumLoanTerm = 365 days
      revert InvalidFixedTerm();
    }

As it is obvious from the above code snippet the fixedTermEndTime < block.timestamp check is performed to ensure that fixedTermEndTime is not less than the current timestamp.

Since the above check is missing in the FixedTermLoanHooks.setFixedTermEndTime function a newFixedTermEndTime could be set which is less than the current timestamp (block.timestamp). As a result the lenders can immediately queue the withdrawals which is not the intended behavior of the fixedTerm hook contract.

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L188-L192

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L238-L240

Recommended Mitigation Steps

Hence it is recommended to update the FixedTermLoanHooks.setFixedTermEndTime function to include the following check.

    if (newFixedTermEndTime < block.timestamp) revert InvalidFixedTerm();

Assessed type

Invalid Validation

laurenceday commented 1 month ago

Duplicate (ish) of https://github.com/code-423n4/2024-08-wildcat-findings/issues/27

d1ll0n commented 1 month ago

The purpose of the check in the constructor is more as a sanity check (since having an end date in the past would make this hooks template useless). If the borrower is updating the date after deployment, setting the end date in the past just means that lenders can immediately withdraw, which would be the desired result in that instance.

3docSec commented 1 month ago

Marking as invalid - intended behavior

c4-judge commented 1 month ago

3docSec marked the issue as unsatisfactory: Invalid