Lack of slippage control on deposit functions

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L117-L120
https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L104-L108
https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarketBase.sol#L406-L465

Vulnerability details

Impact

The lack of slippage control on both deposit functions can lead to a loss of assets for the affected lender.

Vulnerability Details

When a user calls deposit() or depositUpTo() the number of scaled tokens minted is dependent on the current scaleFactor of the market. The scaleFactor increases everytime _getUpdatedState() is called in a new block so the longer it takes to process a user's transaction the less scaledTokens they will receive. If there is high congestion the user may receive far fewer tokens than expected.

The user should therefore be able to pass in a slippage parameter whereby if the amount of scaledTokens they receive is not enough the transaction will be reverted.

    function _getUpdatedState() internal returns (MarketState memory state) {
      state = _state;
      // Handle expired withdrawal batch
      if (state.hasPendingExpiredBatch()) {
        uint256 expiry = state.pendingWithdrawalExpiry;
        // Only accrue interest if time has passed since last update.
        // This will only be false if withdrawalBatchDuration is 0.
        uint32 lastInterestAccruedTimestamp = state.lastInterestAccruedTimestamp;
        if (expiry != lastInterestAccruedTimestamp) {
          (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
            @audit : fees updated here
>>>         .updateScaleFactorAndFees(
              delinquencyFeeBips,
              delinquencyGracePeriod,
              expiry
            );
          emit_InterestAndFeesAccrued(
            lastInterestAccruedTimestamp,
            expiry,
            state.scaleFactor,
            baseInterestRay,
            delinquencyFeeRay,
            protocolFee
          );
        }
        _processExpiredWithdrawalBatch(state);
      }
      uint32 lastInterestAccruedTimestamp = state.lastInterestAccruedTimestamp;
      // Apply interest and fees accrued since last update (expiry or previous tx)
      if (block.timestamp != lastInterestAccruedTimestamp) {
        (uint256 baseInterestRay, uint256 delinquencyFeeRay, uint256 protocolFee) = state
          @audit : fees updated here
>>>       .updateScaleFactorAndFees(
            delinquencyFeeBips,
            delinquencyGracePeriod,
            block.timestamp
          );
        emit_InterestAndFeesAccrued(
          lastInterestAccruedTimestamp,
          block.timestamp,
          state.scaleFactor,
          baseInterestRay,
          delinquencyFeeRay,
          protocolFee
        );
      }

      // more code
    }

POC

• Bob deposits 200 tokens
• At the current scaleFactor he expects to get 170 tokens
• By the time the transaction is processed he only receives 160; 10 less than expected

Tools Used

Manual Review
Foundry Testing

Recommendations

Allow lender to provide a min amount of tokens and a check at the end of execution, such that the transaction will revert if the actual amount of tokens is less than the minimum amount.

Assessed type

MEV

[laurenceday]

Issue identified in previous review by alpeh_v: https://hackmd.io/@geistermeister/BJk4Ekt90 [see bullet point 4].

Report link in the Publicly Known Issues section, but it was visible to all directly underneath it. Anyone could have and should have read it, and factored it in as such.

[3docSec]

While this relates to the finding reported in the audit, I don't think it's quite the same, so I wouldn't consider it out of scope.
While the rate is expected to change over time, this is not an AMM where values can be manipulated to wild extents, and in cases like this, it's widely accepted that a slippage check is not necessary

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b