From 86e76c5332a65795f0cde03e5ff0a8db54500231 Mon Sep 17 00:00:00 2001 From: Timepunk <45543880+0xTimepunk@users.noreply.github.com> Date: Wed, 30 Oct 2024 12:14:22 +0000 Subject: [PATCH] fix: slippage check in wrong order --- Makefile | 2 +- src/router-plus/SuperformRouterPlus.sol | 21 ++--- .../router-plus/SuperformRouterPlus.t.sol | 87 +++++++++++++++++-- 3 files changed, 92 insertions(+), 18 deletions(-) diff --git a/Makefile b/Makefile index 03b2017b0..281b05636 100644 --- a/Makefile +++ b/Makefile @@ -121,7 +121,7 @@ build-sizes: ## Builds the project and shows sizes .PHONY: test-vvv test-vvv: ## Runs tests with verbose output - forge test --match-test test_rebalanceSinglePosition_singleXChainMultiVaultDeposit --evm-version cancun -vvv + forge test --match-contract SuperformRouterPlusTest --evm-version cancun .PHONY: ftest diff --git a/src/router-plus/SuperformRouterPlus.sol b/src/router-plus/SuperformRouterPlus.sol index 02357e423..a3d379e45 100644 --- a/src/router-plus/SuperformRouterPlus.sol +++ b/src/router-plus/SuperformRouterPlus.sol @@ -469,18 +469,18 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus { /// @dev send SPs to router _callSuperformRouter(router_, callData, args.rebalanceFromMsgValue); - uint256 amountToDeposit = interimAsset.balanceOf(address(this)) - args.balanceBefore; + uint256 availableBalanceToDeposit = interimAsset.balanceOf(address(this)) - args.balanceBefore; - if (amountToDeposit == 0) revert Error.ZERO_AMOUNT(); + if (availableBalanceToDeposit == 0) revert Error.ZERO_AMOUNT(); if ( - ENTIRE_SLIPPAGE * amountToDeposit + ENTIRE_SLIPPAGE * availableBalanceToDeposit < ((args.expectedAmountToReceivePostRebalanceFrom * (ENTIRE_SLIPPAGE - args.slippage))) ) { revert Error.VAULT_IMPLEMENTATION_FAILED(); } - uint256 amountIn = _validateAndGetAmountIn(rebalanceToCallData, amountToDeposit); + uint256 amountIn = _validateAndGetAmountIn(rebalanceToCallData, availableBalanceToDeposit); _deposit(router_, interimAsset, amountIn, args.rebalanceToMsgValue, rebalanceToCallData); } @@ -651,7 +651,7 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus { function _validateAndGetAmountIn( bytes calldata rebalanceToCallData, - uint256 amountToDeposit + uint256 availableBalanceToDeposit ) internal view @@ -711,12 +711,13 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus { } } - /// @dev amountIn must be artificially off-chain reduced to be less than amountToDeposit otherwise the approval - /// @dev to transfer tokens to SuperformRouter won't work - if (amountIn > amountToDeposit) revert AMOUNT_IN_NOT_EQUAL_OR_LOWER_THAN_BALANCE(); + /// @dev amountIn must be artificially off-chain reduced to be less than availableBalanceToDeposit otherwise the + /// @dev approval to transfer tokens to SuperformRouter won't work + if (amountIn > availableBalanceToDeposit) revert AMOUNT_IN_NOT_EQUAL_OR_LOWER_THAN_BALANCE(); - /// @dev check against a GLOBAL_SLIPPAGE to prevent a malicious keeper from sending a low amountIn - if (ENTIRE_SLIPPAGE * amountToDeposit < ((amountIn * (ENTIRE_SLIPPAGE - GLOBAL_SLIPPAGE)))) { + /// @dev check amountIn against availableBalanceToDeposit (available balance) via a GLOBAL_SLIPPAGE to prevent a + /// @dev malicious keeper from sending a low amountIn + if (ENTIRE_SLIPPAGE * amountIn < ((availableBalanceToDeposit * (ENTIRE_SLIPPAGE - GLOBAL_SLIPPAGE)))) { revert ASSETS_RECEIVED_OUT_OF_SLIPPAGE(); } } diff --git a/test/unit/router-plus/SuperformRouterPlus.t.sol b/test/unit/router-plus/SuperformRouterPlus.t.sol index 54edf0ff3..ca9ec4106 100644 --- a/test/unit/router-plus/SuperformRouterPlus.t.sol +++ b/test/unit/router-plus/SuperformRouterPlus.t.sol @@ -734,6 +734,78 @@ contract SuperformRouterPlusTest is ProtocolActions { assertEq(SuperPositions(SUPER_POSITIONS_SOURCE).balanceOf(deployer, superformId1), 0); } + function test_revert_AMOUNT_IN_NOT_EQUAL_OR_LOWER_THAN_BALANCE() public { + vm.startPrank(deployer); + + _directDeposit(superformId1, 1e18); + + ISuperformRouterPlus.RebalanceSinglePositionSyncArgs memory args = + _buildRebalanceSinglePositionToOneVaultArgs(deployer); + + SingleVaultSFData memory sfDataRebalanceTo = + abi.decode(_parseCallData(args.rebalanceToCallData), (SingleDirectSingleVaultStateReq)).superformData; + sfDataRebalanceTo.amount = 1e30; + + args.rebalanceToCallData = abi.encodeCall( + IBaseRouter.singleDirectSingleVaultDeposit, SingleDirectSingleVaultStateReq(sfDataRebalanceTo) + ); + SuperPositions(SUPER_POSITIONS_SOURCE).increaseAllowance(ROUTER_PLUS_SOURCE, superformId1, args.sharesToRedeem); + + vm.expectRevert(ISuperformRouterPlus.AMOUNT_IN_NOT_EQUAL_OR_LOWER_THAN_BALANCE.selector); + SuperformRouterPlus(ROUTER_PLUS_SOURCE).rebalanceSinglePosition{ value: 2 ether }(args); + } + + function test_revert_ASSETS_RECEIVED_OUT_OF_SLIPPAGE() public { + vm.startPrank(deployer); + + _directDeposit(superformId1, 1e18); + _directDeposit(superformId2, 1e6); + + (ISuperformRouterPlus.RebalanceMultiPositionsSyncArgs memory args, uint256 totalAmountToDeposit) = + _buildRebalanceTwoPositionsToOneVaultXChainArgs(); + + SingleVaultSFData memory sfDataRebalanceTo = + abi.decode(_parseCallData(args.rebalanceToCallData), (SingleXChainSingleVaultStateReq)).superformData; + + /// @dev keeper attempting to rug the user by reducing amount in + sfDataRebalanceTo.liqRequest.txData = _buildLiqBridgeTxData( + LiqBridgeTxDataArgs( + 1, + args.interimAsset, + getContract(OP, "DAI"), + getContract(OP, "DAI"), + getContract(SOURCE_CHAIN, "SuperformRouter"), + SOURCE_CHAIN, + OP, + OP, + false, + getContract(OP, "CoreStateRegistry"), + uint256(OP), + totalAmountToDeposit - 5e5, + false, + 0, + 1, + 1, + 1, + address(0) + ), + false + ); + + args.rebalanceToCallData = abi.encodeCall( + IBaseRouter.singleXChainSingleVaultDeposit, SingleXChainSingleVaultStateReq(AMBs, OP, sfDataRebalanceTo) + ); + + SuperPositions(SUPER_POSITIONS_SOURCE).increaseAllowance( + ROUTER_PLUS_SOURCE, superformId1, args.sharesToRedeem[0] + ); + SuperPositions(SUPER_POSITIONS_SOURCE).increaseAllowance( + ROUTER_PLUS_SOURCE, superformId2, args.sharesToRedeem[1] + ); + vm.expectRevert(ISuperformRouterPlus.ASSETS_RECEIVED_OUT_OF_SLIPPAGE.selector); + SuperformRouterPlus(ROUTER_PLUS_SOURCE).rebalanceMultiPositions{ value: 2 ether }(args); + } + function test_rebalanceSinglePosition_singleXChainSingleVaultDepositSelector() public { vm.startPrank(deployer); @@ -3189,23 +3261,24 @@ contract SuperformRouterPlusTest is ProtocolActions { args.callData = _callDataRebalanceFromTwoVaults(args.interimAsset); uint256 decimal1 = MockERC20(getContract(SOURCE_CHAIN, "DAI")).decimals(); - uint256 decimal2 = MockERC20(args.interimAsset).decimals(); + uint256 decimal2 = MockERC20(getContract(SOURCE_CHAIN, "USDC")).decimals(); + uint256 decimalInterim = MockERC20(args.interimAsset).decimals(); uint256 previewRedeemAmount1 = IBaseForm(superform1).previewRedeemFrom(args.sharesToRedeem[0]); uint256 expectedAmountToReceivePostRebalanceFrom1; - if (decimal1 > decimal2) { - expectedAmountToReceivePostRebalanceFrom1 = previewRedeemAmount1 / (10 ** (decimal1 - decimal2)); + if (decimal1 > decimalInterim) { + expectedAmountToReceivePostRebalanceFrom1 = previewRedeemAmount1 / (10 ** (decimal1 - decimalInterim)); } else { - expectedAmountToReceivePostRebalanceFrom1 = previewRedeemAmount1 * 10 ** (decimal2 - decimal1); + expectedAmountToReceivePostRebalanceFrom1 = previewRedeemAmount1 * 10 ** (decimalInterim - decimal1); } uint256 previewRedeemAmount2 = IBaseForm(superform2).previewRedeemFrom(args.sharesToRedeem[1]); uint256 expectedAmountToReceivePostRebalanceFrom2; - if (decimal1 > decimal2) { - expectedAmountToReceivePostRebalanceFrom2 = previewRedeemAmount2 / (10 ** (decimal1 - decimal2)); + if (decimal2 > decimalInterim) { + expectedAmountToReceivePostRebalanceFrom2 = previewRedeemAmount2 / (10 ** (decimal2 - decimalInterim)); } else { - expectedAmountToReceivePostRebalanceFrom2 = previewRedeemAmount2 * 10 ** (decimal2 - decimal1); + expectedAmountToReceivePostRebalanceFrom2 = previewRedeemAmount2 * 10 ** (decimalInterim - decimal2); } totalAmountToDeposit = expectedAmountToReceivePostRebalanceFrom1 + expectedAmountToReceivePostRebalanceFrom2;