[OZ:C-01] rework cancel flow

.changeset/heavy-gorillas-watch.md

@@ -0,0 +1,5 @@
+---
+"@exactly/protocol": patch
+---
+
+🔒 escrow: validate streams on cancel and withdraw

.changeset/thin-bugs-do.md

@@ -0,0 +1,5 @@
+---
+"@exactly/protocol": patch
+---
+
+🚸 escrow: return reserve on external stream cancel

.gas-snapshot

@@ -93,35 +93,40 @@ DebtPreviewerTest:testPreviewLeverageSameWETHAssetMaxRatioMultipleCollateralAndD
 DebtPreviewerTest:testPreviewLeverageSameWETHAssetMultipleCollateralAndDebtWithMinHealthFactor() (gas: 1708472)
 DebtPreviewerTest:testPreviewMaxRatioWithdrawWithSameAssetLeverage() (gas: 1020516)
 DebtPreviewerTest:testPreviewSameAssetInvalidLeverageShouldCapRatio() (gas: 910754)
-EscrowedEXATest:testCancelFromStreamAndGetReserveBack() (gas: 457085)
-EscrowedEXATest:testCancelFromStreamJustCreated() (gas: 412309)
-EscrowedEXATest:testCancelShouldDeleteReserves() (gas: 470857)
-EscrowedEXATest:testCancelShouldGiveReservesBack() (gas: 676123)
-EscrowedEXATest:testCancelTwiceShouldRevert() (gas: 470361)
-EscrowedEXATest:testCancelWithInvalidAccount() (gas: 348019)
-EscrowedEXATest:testGrantTransferrerRoleAsAdmin() (gas: 45901)
-EscrowedEXATest:testMint() (gas: 165141)
+EscrowedEXATest:testCancelExternalStreams() (gas: 452302)
+EscrowedEXATest:testCancelExternalStreamsWithesEXACancel() (gas: 903873)
+EscrowedEXATest:testCancelFromStreamAndGetReserveBack() (gas: 451714)
+EscrowedEXATest:testCancelFromStreamJustCreated() (gas: 404796)
+EscrowedEXATest:testCancelShouldDeleteReserves() (gas: 471143)
+EscrowedEXATest:testCancelShouldGiveReservesBack() (gas: 676562)
+EscrowedEXATest:testCancelTwiceShouldRevert() (gas: 456028)
+EscrowedEXATest:testCancelWithInvalidAccount() (gas: 348283)
+EscrowedEXATest:testFakeTokenWithesEXARecipient() (gas: 932349)
+EscrowedEXATest:testGrantTransferrerRoleAsAdmin() (gas: 45812)
+EscrowedEXATest:testMint() (gas: 165163)
 EscrowedEXATest:testMintMoreThanBalance() (gas: 27564)
-EscrowedEXATest:testMintToAnother() (gas: 167337)
-EscrowedEXATest:testMintZero() (gas: 14163)
-EscrowedEXATest:testRedeemAsNotRedeemer() (gas: 214641)
+EscrowedEXATest:testMintToAnother() (gas: 167359)
+EscrowedEXATest:testMintZero() (gas: 14185)
+EscrowedEXATest:testRedeemAsNotRedeemer() (gas: 214783)
 EscrowedEXATest:testRedeemAsRedeemer() (gas: 165497)
-EscrowedEXATest:testRedeemAsRedeemerToAnother() (gas: 170381)
-EscrowedEXATest:testSetReserveRatioAsAdmin() (gas: 24450)
+EscrowedEXATest:testRedeemAsRedeemerToAnother() (gas: 170399)
+EscrowedEXATest:testSetReserveRatioAsAdmin() (gas: 24495)
 EscrowedEXATest:testSetReserveRatioAsNotAdmin() (gas: 47318)
-EscrowedEXATest:testSetVestingPeriodAsAdmin() (gas: 24598)
+EscrowedEXATest:testSetReserveRatioAsZero() (gas: 16293)
+EscrowedEXATest:testSetVestingPeriodAsAdmin() (gas: 24642)
 EscrowedEXATest:testSetVestingPeriodAsNotAdmin() (gas: 47411)
-EscrowedEXATest:testTransferToNotTransferrer() (gas: 174775)
-EscrowedEXATest:testTransferToTransferrer() (gas: 210353)
-EscrowedEXATest:testVest() (gas: 346856)
-EscrowedEXATest:testVestToAnother() (gas: 403671)
-EscrowedEXATest:testVestToAnotherAndCancel() (gas: 489338)
-EscrowedEXATest:testVestWithPermitReserve() (gas: 458552)
+EscrowedEXATest:testTransferToNotTransferrer() (gas: 174841)
+EscrowedEXATest:testTransferToTransferrer() (gas: 210463)
+EscrowedEXATest:testVest() (gas: 346917)
+EscrowedEXATest:testVestToAnother() (gas: 403826)
+EscrowedEXATest:testVestToAnotherAndCancel() (gas: 489580)
+EscrowedEXATest:testVestWithPermitReserve() (gas: 458876)
 EscrowedEXATest:testVestZero() (gas: 14151)
-EscrowedEXATest:testWithdrawFromStreamAndGetReserveBack() (gas: 329552)
-EscrowedEXATest:testWithdrawMaxFromMultipleStreams() (gas: 1006649)
-EscrowedEXATest:testWithdrawMaxShouldGiveReserveBackWhenDepleted() (gas: 327542)
-EscrowedEXATest:testWithdrawMaxWithInvalidSender() (gas: 339414)
+EscrowedEXATest:testWithdrawFromStreamAndGetReserveBack() (gas: 329850)
+EscrowedEXATest:testWithdrawFromUnknownStream() (gas: 898481)
+EscrowedEXATest:testWithdrawMaxFromMultipleStreams() (gas: 1007617)
+EscrowedEXATest:testWithdrawMaxShouldGiveReserveBackWhenDepleted() (gas: 327862)
+EscrowedEXATest:testWithdrawMaxWithInvalidSender() (gas: 339700)
 InterestRateModelTest:testFixedBorrowRate() (gas: 8089)
 InterestRateModelTest:testFloatingBorrowRate() (gas: 6236)
 InterestRateModelTest:testMinFixedRate() (gas: 7610)

contracts/periphery/EscrowedEXA.sol

@@ -87,7 +87,7 @@ contract EscrowedEXA is ERC20VotesUpgradeable, AccessControlUpgradeable {
   function vest(uint128 amount, address to) public returns (uint256 streamId) {
     assert(amount != 0);
     _burn(msg.sender, amount);
-    uint256 reserve = amount.mulWadDown(reserveRatio);
+    uint256 reserve = amount.mulWadUp(reserveRatio);
     exa.safeTransferFrom(msg.sender, address(this), reserve);
     streamId = sablier.createWithDurations(
       CreateWithDurations({
@@ -131,6 +131,7 @@ contract EscrowedEXA is ERC20VotesUpgradeable, AccessControlUpgradeable {
   function _cancel(uint256[] memory streamIds) internal 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];
@@ -146,6 +147,7 @@ contract EscrowedEXA is ERC20VotesUpgradeable, AccessControlUpgradeable {
   function withdrawMax(uint256[] memory streamIds) public {
     for (uint256 i = 0; i < streamIds.length; ++i) {
       uint256 streamId = streamIds[i];
+      checkStream(streamId);
       assert(msg.sender == sablier.getRecipient(streamId));
       withdrawMax(streamId);
     }
@@ -155,20 +157,34 @@ contract EscrowedEXA is ERC20VotesUpgradeable, AccessControlUpgradeable {
   /// @param streamId streamId to withdraw from.
   function withdrawMax(uint256 streamId) internal {
     if (sablier.withdrawableAmountOf(streamId) != 0) sablier.withdrawMax(streamId, msg.sender);
-    if (sablier.isDepleted(streamId)) {
-      uint256 reserve = reserves[streamId];
-      delete reserves[streamId];
-      exa.safeTransfer(msg.sender, reserve);
-    }
+    if (sablier.isDepleted(streamId)) returnReserve(streamId, msg.sender);
+  }
+
+  /// @notice Checks if a stream is valid through its reserve. Reverts with `InvalidStream` if it is not.
+  /// @param streamId streamId to check.
+  function checkStream(uint256 streamId) internal view {
+    if (reserves[streamId] == 0) revert InvalidStream();
+  }
+
+  /// @notice Returns the reserve to the recipient.
+  /// @param streamId streamId of the reserve to return.
+  /// @param recipient recipient of the reserve.
+  function returnReserve(uint256 streamId, address recipient) internal {
+    uint256 reserve = reserves[streamId];
+    delete reserves[streamId];
+    exa.safeTransfer(recipient, reserve);
   }
 
   /// @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 sent to the recipient.
-  function onStreamCanceled(uint256, address recipient, uint128 senderAmount, uint128) external {
+  /// @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);
   }
 
   /// @notice Sets the vesting period.
@@ -183,6 +199,7 @@ contract EscrowedEXA is ERC20VotesUpgradeable, AccessControlUpgradeable {
   /// @param reserveRatio_ New reserve ratio.
   /// @dev Caller must have DEFAULT_ADMIN_ROLE.
   function setReserveRatio(uint256 reserveRatio_) public onlyRole(DEFAULT_ADMIN_ROLE) {
+    assert(reserveRatio_ != 0);
     reserveRatio = reserveRatio_;
     emit ReserveRatioSet(reserveRatio_);
   }
@@ -207,6 +224,7 @@ contract EscrowedEXA is ERC20VotesUpgradeable, AccessControlUpgradeable {
 }
 
 error Untransferable();
+error InvalidStream();
 
 interface ISablierV2LockupLinear {
   function cancel(uint256 streamId) external;

test/EscrowedEXA.t.sol

@@ -7,10 +7,15 @@ import { ForkTest, stdError } from "./Fork.t.sol";
 import {
   EXA,
   Permit,
+  Broker,
+  Durations,
   EscrowedEXA,
+  InvalidStream,
   Untransferable,
+  CreateWithDurations,
   ISablierV2LockupLinear
 } from "../contracts/periphery/EscrowedEXA.sol";
+import { MockERC20 } from "solmate/src/test/utils/mocks/MockERC20.sol";
 
 contract EscrowedEXATest is ForkTest {
   using FixedPointMathLib for uint256;
@@ -186,7 +191,7 @@ contract EscrowedEXATest is ForkTest {
     assertEq(exa.balanceOf(address(this)), initialEXA + withdrawableAmount, "should give reserves back");
     assertEq(esEXA.reserves(streamId1), 0, "reserves[streamId1] == 0");
     assertEq(esEXA.reserves(streamId2), 0, "reserves[streamId2] == 0");
-    assertEq(esEXA.balanceOf(address(this)), esEXABefore + initialAmount, "should give back half of the esexa");
+    assertEq(esEXA.balanceOf(address(this)), esEXABefore + initialAmount, "should give back half of the esEXA");
   }
 
   function testVestToAnotherAndCancel() external {
@@ -221,7 +226,7 @@ contract EscrowedEXATest is ForkTest {
     esEXA.cancel(streamIds);
 
     assertEq(esEXA.reserves(streamId), 0, "reserves[streamId] == 0");
-    assertEq(esEXA.balanceOf(address(this)), esEXABefore + amount / 2, "should give back half of the esexa");
+    assertEq(esEXA.balanceOf(address(this)), esEXABefore + amount / 2, "should give back half of the esEXA");
     assertEq(exa.balanceOf(address(this)), exaBefore + reserve + withdrawableAmount, "should give back reserve");
   }
 
@@ -235,7 +240,7 @@ contract EscrowedEXATest is ForkTest {
     uint256[] memory streamIds = new uint256[](1);
     streamIds[0] = streamId;
     esEXA.cancel(streamIds);
-    vm.expectRevert(abi.encodeWithSelector(SablierV2Lockup_StreamDepleted.selector, streamId));
+    vm.expectRevert(abi.encodeWithSelector(InvalidStream.selector));
     esEXA.cancel(streamIds);
   }
 
@@ -411,18 +416,17 @@ contract EscrowedEXATest is ForkTest {
     uint256 amount = 1_000 ether;
     uint256 reserve = amount.mulWadDown(esEXA.reserveRatio());
     esEXA.mint(amount, address(this));
-    uint256[] memory streams = new uint256[](1);
-    streams[0] = esEXA.vest(uint128(amount), address(this));
+    uint256 streamId = esEXA.vest(uint128(amount), address(this));
 
     uint256 exaBefore = exa.balanceOf(address(this));
     vm.warp(block.timestamp + esEXA.vestingPeriod() / 2);
 
-    esEXA.sablier().cancel(streams[0]);
-
-    assertEq(esEXA.balanceOf(address(this)), amount / 2, "half esEXA sholud be back");
+    esEXA.sablier().cancel(streamId);
 
-    esEXA.withdrawMax(streams);
-    assertEq(exa.balanceOf(address(this)), exaBefore + amount / 2 + reserve, "amount/2 + reserve should be back");
+    assertEq(esEXA.balanceOf(address(this)), amount / 2, "half esEXA should be back");
+    assertEq(exa.balanceOf(address(this)), exaBefore + reserve, "reserve should be back");
+    esEXA.sablier().withdrawMax(streamId, address(this));
+    assertEq(exa.balanceOf(address(this)), exaBefore + reserve + amount / 2, "amount/2 should be back");
   }
 
   function testCancelFromStreamJustCreated() external {
@@ -436,10 +440,119 @@ contract EscrowedEXATest is ForkTest {
 
     esEXA.sablier().cancel(streams[0]);
 
-    assertEq(esEXA.balanceOf(address(this)), amount, "amount esEXA sholud be back");
+    assertEq(esEXA.balanceOf(address(this)), amount, "amount esEXA should be back");
+    assertEq(exa.balanceOf(address(this)), exaBefore + reserve, "reserve should be back");
+  }
+
+  function testFakeTokenWithesEXARecipient() external {
+    MockERC20 token = new MockERC20("Malicious token", "MT", 18);
+    uint128 amount = 1_000 ether;
+    token.mint(address(this), amount);
+    token.approve(address(sablier), type(uint256).max);
 
+    uint256 esEXABefore = esEXA.balanceOf(address(this));
+    uint256 streamId = sablier.createWithDurations(
+      CreateWithDurations({
+        asset: EXA(address(token)),
+        sender: address(this),
+        recipient: address(esEXA),
+        totalAmount: amount,
+        cancelable: true,
+        durations: Durations({ cliff: 0, total: 1 }),
+        broker: Broker({ account: address(0), fee: 0 })
+      })
+    );
+    sablier.cancel(streamId);
+
+    assertEq(esEXA.balanceOf(address(this)), esEXABefore, "esEXA balance shouldn't change");
+  }
+
+  function testCancelExternalStreams() external {
+    uint128 amount = 1_000 ether;
+    exa.approve(address(sablier), type(uint256).max);
+    uint256 esEXABefore = esEXA.balanceOf(address(this));
+    uint256 streamId = sablier.createWithDurations(
+      CreateWithDurations({
+        asset: exa,
+        sender: address(this),
+        recipient: address(esEXA),
+        totalAmount: amount,
+        cancelable: true,
+        durations: Durations({ cliff: 0, total: 1 }),
+        broker: Broker({ account: address(0), fee: 0 })
+      })
+    );
+    sablier.cancel(streamId);
+    assertEq(esEXA.balanceOf(address(this)), esEXABefore, "esEXA balance shouldn't change");
+
+    streamId = sablier.createWithDurations(
+      CreateWithDurations({
+        asset: exa,
+        sender: address(esEXA),
+        recipient: address(this),
+        totalAmount: amount,
+        cancelable: true,
+        durations: Durations({ cliff: 0, total: 1 }),
+        broker: Broker({ account: address(0), fee: 0 })
+      })
+    );
+    uint256 exaBefore = exa.balanceOf(address(this));
+    sablier.cancel(streamId);
+    assertEq(exa.balanceOf(address(this)), exaBefore, "EXA balance shouldn't change");
+    assertEq(esEXA.balanceOf(address(this)), esEXABefore, "should not mint esEXA");
+  }
+
+  function testCancelExternalStreamsWithesEXACancel() external {
+    MockERC20 token = new MockERC20("Malicious token", "MT", 18);
+    uint128 amount = 1_000 ether;
+    token.mint(address(this), amount);
+    token.approve(address(sablier), type(uint256).max);
+
+    uint256 esEXABefore = esEXA.balanceOf(address(this));
+    uint256[] memory streams = new uint256[](1);
+    streams[0] = sablier.createWithDurations(
+      CreateWithDurations({
+        asset: EXA(address(token)),
+        sender: address(esEXA),
+        recipient: address(this),
+        totalAmount: amount,
+        cancelable: true,
+        durations: Durations({ cliff: 0, total: 1 }),
+        broker: Broker({ account: address(0), fee: 0 })
+      })
+    );
+
+    vm.expectRevert(InvalidStream.selector);
+    esEXA.cancel(streams);
+    assertEq(esEXA.balanceOf(address(this)), esEXABefore, "esEXA balance shouldn't change");
+  }
+
+  function testSetReserveRatioAsZero() external {
+    vm.expectRevert(stdError.assertionError);
+    esEXA.setReserveRatio(0);
+  }
+
+  function testWithdrawFromUnknownStream() external {
+    MockERC20 token = new MockERC20("Random token", "RNT", 18);
+    uint128 amount = 1_000 ether;
+    token.mint(address(this), amount);
+    token.approve(address(sablier), type(uint256).max);
+
+    uint256[] memory streams = new uint256[](1);
+    streams[0] = sablier.createWithDurations(
+      CreateWithDurations({
+        asset: EXA(address(token)),
+        sender: address(this),
+        recipient: address(this),
+        totalAmount: amount,
+        cancelable: true,
+        durations: Durations({ cliff: 0, total: 1 }),
+        broker: Broker({ account: address(0), fee: 0 })
+      })
+    );
+
+    vm.expectRevert(abi.encodeWithSelector(InvalidStream.selector));
     esEXA.withdrawMax(streams);
-    assertEq(exa.balanceOf(address(this)), exaBefore + reserve, "reserve should be back");
   }
 
   event ReserveRatioSet(uint256 reserveRatio);