howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/market/WildcatMarketBase.sol#L406

Vulnerability details

Impact

Borrower can bypass calls to the onRepay hook

Proof of Concept

Within the protocol, users are free to implement markets with a set of hooks, including a onRepay hook which is intended to be called any time a repayment is made.

  function _repay(MarketState memory state, uint256 amount, uint256 baseCalldataSize) internal {
    if (amount == 0) revert_NullRepayAmount();
    if (state.isClosed) revert_RepayToClosedMarket();

    asset.safeTransferFrom(msg.sender, address(this), amount);
    emit_DebtRepaid(msg.sender, amount);

    // Execute repay hook if enabled
    hooks.onRepay(amount, state, baseCalldataSize);
  }

However, the problem is that any funds transferred directly to the contract are treated as a repayment by the borrower. This does allow the borrower, any time they wish to make a repayment, they can just send the funds to the contract and avoid the onRepay hook (as it may apply restrictions, extra fees or generally - anything)

This basically makes the onRepay hook useless.

Tools Used

Manual review

Recommended Mitigation Steps

Consider either using internal accounting or removing the onRepay hook.

Assessed type

Context

laurenceday commented 1 month ago

I was speaking to Dillon about this, and while the hook is there for consistency (given that the major functions each have one), in practice I can't think of any kind of hook behaviour we'd actually want to enforce on this, so it's somewhat moot [and the filing recognises this given that one of the mitigations is just 'lose it'].

With that said, I've mentioned in a few other places that repay is a bit of a strange duck compared to the others because it macros functionality that we can't avoid and is only really there as a QOL improvement.

Will come back to this one, but leaning towards an acknowledge.

d1ll0n commented 3 weeks ago

I wouldn't consider this a medium as the onRepay hook is intended to be used when the repay function is actually used, not just any time the market is updated after having received tokens (in which case we have no way of knowing who sent it). Still a decent QA finding for pointing out that onRepay is largely useless though.

3docSec commented 3 weeks ago

It appears to me that hooks are set by the borrower at market deployment. So the borrower can repay debt and bypass their own hooks; looks indeed more like a QA than an M finding.

c4-judge commented 3 weeks ago

3docSec changed the severity to QA (Quality Assurance)