`onExecuteWithdrawal` hook uses an incorrect function signature definition

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/types/HooksConfig.sol#L374-L426 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketWithdrawals.sol#L255 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L133 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/FixedTermLoanHooks.sol#L873

Vulnerability details

Impact

onExecuteWithdrawal function is incorrectly encoded in HooksConfig.sol and as a result, transactions that use the hook will not be successfully executed.

Proof of Concept

From the IHooks interface, onExecuteWithdrawal is defined as follows using a uint128 normalizedAmountWithdrawn parameter.

  function onExecuteWithdrawal(
    address lender,
    uint128 normalizedAmountWithdrawn,
    MarketState calldata intermediateState,
    bytes calldata extraData
  ) external virtual;

However, when encoding in HooksConfig.sol, we see that a uint256 scaledAmount parameter is passed in instead.

  function onExecuteWithdrawal(
    HooksConfig self,
    address lender,
    uint256 scaledAmount,
    MarketState memory state,
    uint256 baseCalldataSize
  ) internal {

Also, while encoding the parameters, the onExecuteWithdrawal selector is gotten using the interface, but the uint256 scaled amount offset is written into the call data instead.

    address target = self.hooksAddress();
    uint32 onExecuteWithdrawalSelector = uint32(IHooks.onExecuteWithdrawal.selector); //@note
    if (self.useOnExecuteWithdrawal()) {
      assembly {
        let extraCalldataBytes := sub(calldatasize(), baseCalldataSize)
        let cdPointer := mload(0x40)
        let headPointer := add(cdPointer, 0x20)
        // Write selector for `onExecuteWithdrawal`
        mstore(cdPointer, onExecuteWithdrawalSelector)
        // Write `lender` to hook calldata
        mstore(headPointer, lender)
        // Write `scaledAmount` to hook calldata
        mstore(add(headPointer, ExecuteWithdrawalHook_ScaledAmount_Offset), scaledAmount) //@note
        // Copy market state to hook calldata
        mcopy(add(headPointer, ExecuteWithdrawalHook_State_Offset), state, M

This causes that the defined function selector be different from its expected arguments and as such the hook fails.

Tools Used

Manual code review

Recommended Mitigation Steps

  uint256 internal constant ExecuteWithdrawalHook_Base_Size = 0x0244;
- uint256 internal constant ExecuteWithdrawalHook_ScaledAmount_Offset = 0x20;
+ uint128 internal constant ExecuteWithdrawalHook_NormalizedAmount_Offset = 0x20;

//...
  function onExecuteWithdrawal(
    HooksConfig self,
    address lender,
-   uint256 scaledAmount,
+   uint256 normalizedAmount,

//...
        // Write `scaledAmount` to hook calldata
-       mstore(add(headPointer, ExecuteWithdrawalHook_ScaledAmount_Offset), scaledAmount)
+       mstore(add(headPointer, ExecuteWithdrawalHook_ScaledAmount_Offset), normalizedAmount)

Assessed type

Context

[d1ll0n]

This is incorrect - the signature of the internal function is different because the base calldatasize is used to derive the offset to extraData which then gets passed in to the external function call. Also the suggested solution is just a naming fix, it's unrelated to the issue you described.

[3docSec]

The integration can be verified with this test - which works for me.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid