cryptic - `EscrowedEXA::onStreamCanceled` callback calls `checkStream()` on deleted stream, causing the callback to always fail

[sherlock-admin4]

cryptic

high

EscrowedEXA::onStreamCanceled callback calls checkStream() on deleted stream, causing the callback to always fail

Summary

EscrowedEXA contains functionality for vesting EXA tokens, and allows the recipient to cancel the vesting stream. When cancelling a vesting steam, EscrowedEXA contract implements onStreamCanceled callback to mint esEXA to the recipient with the remaining EXA received from the cancelled stream.

The problem is that the onStreamCanceled attempts to call checkStream() on deleted streams, causing it to silently fail on every call, and the caller will not receive the remaining esEXA tokens.

Vulnerability Detail

When cancelling a vesting stream the following function is initiated:

EscrowedEXA::cancel https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L133

  function cancel(uint256[] memory streamIds) external returns (uint256 streamsReserves) {
    uint128 refundableAmount;
    for (uint256 i = 0; i < streamIds.length; ++i) {
      uint256 streamId = streamIds[i];
      checkStream(streamId);
      assert(msg.sender == sablier.getRecipient(streamId));
      streamsReserves += reserves[streamId];
@>    delete reserves[streamId];
      refundableAmount += sablier.refundableAmountOf(streamId);
      withdrawMax(streamId);
@>    sablier.cancel(streamId);
    }
    emit Cancel(msg.sender, streamIds);
    _mint(msg.sender, refundableAmount);
    exa.safeTransfer(msg.sender, streamsReserves);
  }

Accounting of streamsReserves is kept, which corresponds to the amount of EXA tokens that is sent to the caller upon cancelling. Note that reserves[streamId] is deleted.

When sablier.cancel(streamId) is called, the stream is cancelled via the Sablier external contract, which initiates the onStreamCanceled callback.

EscrowedEXA contract implements this function to mint esEXA to the recipient with the remaining EXA received from the cancelled stream.

EscrowedEXA::onStreamCanceled https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L188

  /// @notice Hook called when a recipient cancels a stream.
  /// @notice Mints esEXA to the recipient with the remaining EXA received from the canceled stream.
  /// @param streamId streamId of the cancelled stream.
  /// @param recipient recipient of the cancelled stream.
  /// @param senderAmount amount of EXA received back from the stream cancelling.
  function onStreamCanceled(uint256 streamId, address recipient, uint128 senderAmount, uint128) external {
    assert(msg.sender == address(sablier));
@>  checkStream(streamId);
    _mint(recipient, senderAmount);
    returnReserve(streamId, recipient);
  }

Here a call to checkStream() is made with the same streamId.

EscrowedEXA::checkStream https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L170

  function checkStream(uint256 streamId) internal view {
    if (reserves[streamId] == 0) revert InvalidStream();
  }

However, recall that reserves[streamId] was set to 0 when delete reserves[streamId] was executed prior to the sablier.cancel(streamId) call. Therefore this will revert on every call. It is important to note that the entire call will not revert because Sablier external contract implements a try/catch block to ensure cancelling streams does not revert if the callback fails:

SablierV2LockupLinear::_cancel https://etherscan.io/address/0xafb979d9afad1ad27c5eff4e27226e3ab9e5dcc9#code

    function _cancel(uint256 streamId) internal override {
       .
       .
       .
        // @audit this call will not revert when `onStreamCanceled` reverts due to try/catch
         if (recipient.code.length > 0) {
            try ISablierV2LockupRecipient(recipient).onStreamCanceled({
                streamId: streamId,
                sender: sender,
                senderAmount: senderAmount,
                recipientAmount: recipientAmount
            }) { } catch { }
        }

    }

Impact

EscrowedEXA::onStreamCanceled callback will always fail silently, recipient will not receive the esEXA tokens from the remaining EXA received by cancelling the stream as intended by the callback

Code Snippet

https://github.com/sherlock-audit/2024-04-interest-rate-model/blob/main/protocol/contracts/periphery/EscrowedEXA.sol#L133

Tool used

Manual Review

Recommendation

Consider removing the call to checkStream() when callback is executed:

  function onStreamCanceled(uint256 streamId, address recipient, uint128 senderAmount, uint128) external {
    assert(msg.sender == address(sablier));
-   checkStream(streamId);
    _mint(recipient, senderAmount);
    returnReserve(streamId, recipient);
  }

[santipu03]

This issue is invalid because the callback onStreamCanceled will only be called if the stream was canceled directly through Sablier and not via EscrowedEXA::cancel.