Skip to content

Commit 1bf66a6

Browse files
0xTimepunk0xTimepunk
committed
fix: routerplus issue
1 parent 07f6df7 commit 1bf66a6

File tree

3 files changed

+154
-3
lines changed

3 files changed

+154
-3
lines changed

‎Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ build-sizes: ## Builds the project and shows sizes
121121

122122
.PHONY: test-vvv
123123
test-vvv: ## Runs tests with verbose output
124-
forge test --match-test test_superRegistryAddresses --evm-version cancun -vv
124+
forge test --match-test test_deposit4626_maliciousReceiver --evm-version cancun -vvv
125125

126126
.PHONY: ftest
127127
ftest: ## Runs tests with cancun evm version

‎src/router-plus/SuperformRouterPlus.sol

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,13 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
3333
/// @dev Tolerance constant to account for tokens with rounding issues on transfer
3434
uint256 constant TOLERANCE_CONSTANT = 10 wei;
3535

36+
//////////////////////////////////////////////////////////////
37+
// ERRORS //
38+
//////////////////////////////////////////////////////////////
39+
40+
/// @notice thrown if the receiver address is invalid
41+
/// @dev notice this error was added to prevent malicious deposits
42+
error RECEIVER_ADDRESS_MISMATCH();
3643
//////////////////////////////////////////////////////////////
3744
// CONSTRUCTOR //
3845
//////////////////////////////////////////////////////////////
@@ -485,7 +492,8 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
485492
revert Error.VAULT_IMPLEMENTATION_FAILED();
486493
}
487494

488-
uint256 amountIn = _validateAndGetAmountIn(rebalanceToCallData, availableBalanceToDeposit);
495+
uint256 amountIn =
496+
_validateAndGetAmountIn(rebalanceToCallData, args.receiverAddressSP, availableBalanceToDeposit);
489497

490498
_deposit(router_, interimAsset, amountIn, args.rebalanceToMsgValue, rebalanceToCallData);
491499
}
@@ -643,7 +651,7 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
643651

644652
uint256 amountRedeemed = _redeemShare(vault, assetAdr, args.amount, args.expectedOutputAmount, args.maxSlippage);
645653

646-
uint256 amountIn = _validateAndGetAmountIn(args.depositCallData, amountRedeemed);
654+
uint256 amountIn = _validateAndGetAmountIn(args.depositCallData, args.receiverAddressSP, amountRedeemed);
647655

648656
address router = _getAddress(keccak256("SUPERFORM_ROUTER"));
649657

@@ -656,6 +664,7 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
656664

657665
function _validateAndGetAmountIn(
658666
bytes calldata rebalanceToCallData,
667+
address receiverAddressSP,
659668
uint256 availableBalanceToDeposit
660669
)
661670
internal
@@ -674,10 +683,12 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
674683
SingleVaultSFData memory sfData =
675684
abi.decode(_parseCallData(rebalanceToCallData), (SingleDirectSingleVaultStateReq)).superformData;
676685
amountIn = _takeAmountIn(sfData.liqRequest, sfData.amount);
686+
_checkReceiverAddress(receiverAddressSP, sfData.receiverAddress, sfData.receiverAddressSP);
677687
} else if (rebalanceToSelector == IBaseRouter.singleXChainSingleVaultDeposit.selector) {
678688
SingleVaultSFData memory sfData =
679689
abi.decode(_parseCallData(rebalanceToCallData), (SingleXChainSingleVaultStateReq)).superformData;
680690
amountIn = _takeAmountIn(sfData.liqRequest, sfData.amount);
691+
_checkReceiverAddress(receiverAddressSP, sfData.receiverAddress, sfData.receiverAddressSP);
681692
} else if (rebalanceToSelector == IBaseRouter.singleDirectMultiVaultDeposit.selector) {
682693
MultiVaultSFData memory sfData =
683694
abi.decode(_parseCallData(rebalanceToCallData), (SingleDirectMultiVaultStateReq)).superformData;
@@ -687,6 +698,7 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
687698
amountInTemp = _takeAmountIn(sfData.liqRequests[i], sfData.amounts[i]);
688699
amountIn += amountInTemp;
689700
}
701+
_checkReceiverAddress(receiverAddressSP, sfData.receiverAddress, sfData.receiverAddressSP);
690702
} else if (rebalanceToSelector == IBaseRouter.singleXChainMultiVaultDeposit.selector) {
691703
MultiVaultSFData memory sfData =
692704
abi.decode(_parseCallData(rebalanceToCallData), (SingleXChainMultiVaultStateReq)).superformsData;
@@ -695,13 +707,15 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
695707
amountInTemp = _takeAmountIn(sfData.liqRequests[i], sfData.amounts[i]);
696708
amountIn += amountInTemp;
697709
}
710+
_checkReceiverAddress(receiverAddressSP, sfData.receiverAddress, sfData.receiverAddressSP);
698711
} else if (rebalanceToSelector == IBaseRouter.multiDstSingleVaultDeposit.selector) {
699712
SingleVaultSFData[] memory sfData =
700713
abi.decode(_parseCallData(rebalanceToCallData), (MultiDstSingleVaultStateReq)).superformsData;
701714
uint256 lenDst = sfData.length;
702715
for (uint256 i; i < lenDst; ++i) {
703716
amountInTemp = _takeAmountIn(sfData[i].liqRequest, sfData[i].amount);
704717
amountIn += amountInTemp;
718+
_checkReceiverAddress(receiverAddressSP, sfData[i].receiverAddress, sfData[i].receiverAddressSP);
705719
}
706720
} else if (rebalanceToSelector == IBaseRouter.multiDstMultiVaultDeposit.selector) {
707721
MultiVaultSFData[] memory sfData =
@@ -713,6 +727,7 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
713727
amountInTemp = _takeAmountIn(sfData[i].liqRequests[j], sfData[i].amounts[j]);
714728
amountIn += amountInTemp;
715729
}
730+
_checkReceiverAddress(receiverAddressSP, sfData[i].receiverAddress, sfData[i].receiverAddressSP);
716731
}
717732
}
718733

@@ -726,4 +741,21 @@ contract SuperformRouterPlus is ISuperformRouterPlus, BaseSuperformRouterPlus {
726741
revert ASSETS_RECEIVED_OUT_OF_SLIPPAGE();
727742
}
728743
}
744+
745+
function _checkReceiverAddress(
746+
address receiverAddressSP,
747+
address callDataReceiverAddress,
748+
address callDataReceiverAddressSP
749+
)
750+
internal
751+
pure
752+
{
753+
/// @dev These checks below prevent a user approving funds to router plus while another user receives the
754+
/// SuperPositions
755+
756+
/// @dev We force all receiver addresses to match
757+
if (receiverAddressSP != callDataReceiverAddressSP || callDataReceiverAddressSP != callDataReceiverAddress) {
758+
revert RECEIVER_ADDRESS_MISMATCH();
759+
}
760+
}
729761
}

‎test/unit/router-plus/SuperformRouterPlus.t.sol

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2747,6 +2747,73 @@ contract SuperformRouterPlusTest is ProtocolActions {
27472747
// SAME_CHAIN REBALANCING TESTS //
27482748
//////////////////////////////////////////////////////////////
27492749

2750+
function _buildRebalanceSinglePositionToOneVaultArgsHack(address user)
2751+
internal
2752+
view
2753+
returns (ISuperformRouterPlus.RebalanceSinglePositionSyncArgs memory args)
2754+
{
2755+
args.id = superformId1;
2756+
args.sharesToRedeem = SuperPositions(SUPER_POSITIONS_SOURCE).balanceOf(user, superformId1);
2757+
args.rebalanceFromMsgValue = 1 ether;
2758+
args.rebalanceToMsgValue = 1 ether;
2759+
args.interimAsset = getContract(SOURCE_CHAIN, "USDC");
2760+
args.slippage = 100;
2761+
args.receiverAddressSP = user;
2762+
args.callData = _callDataRebalanceFrom(args.interimAsset);
2763+
2764+
uint256 decimal1 = MockERC20(getContract(SOURCE_CHAIN, "DAI")).decimals();
2765+
uint256 decimal2 = MockERC20(args.interimAsset).decimals();
2766+
uint256 previewRedeemAmount = IBaseForm(superform1).previewRedeemFrom(args.sharesToRedeem);
2767+
2768+
if (decimal1 > decimal2) {
2769+
args.expectedAmountToReceivePostRebalanceFrom = previewRedeemAmount / (10 ** (decimal1 - decimal2));
2770+
} else {
2771+
args.expectedAmountToReceivePostRebalanceFrom = previewRedeemAmount * 10 ** (decimal2 - decimal1);
2772+
}
2773+
2774+
args.rebalanceToCallData = _callDataRebalanceToOneVaultSameChainHack(
2775+
args.expectedAmountToReceivePostRebalanceFrom, args.interimAsset, user
2776+
);
2777+
}
2778+
2779+
function _callDataRebalanceToOneVaultSameChainHack(
2780+
uint256 amountToDeposit,
2781+
address interimToken,
2782+
address user
2783+
)
2784+
internal
2785+
view
2786+
returns (bytes memory)
2787+
{
2788+
SingleVaultSFData memory data = SingleVaultSFData(
2789+
superformId2,
2790+
amountToDeposit,
2791+
IBaseForm(superform2).previewDepositTo(amountToDeposit),
2792+
100,
2793+
LiqRequest("", interimToken, address(0), 1, SOURCE_CHAIN, 0),
2794+
"",
2795+
false,
2796+
false,
2797+
address(0xDEAD),
2798+
address(0xDEAD),
2799+
""
2800+
);
2801+
return abi.encodeCall(IBaseRouter.singleDirectSingleVaultDeposit, SingleDirectSingleVaultStateReq(data));
2802+
}
2803+
2804+
function test_rebalanceFromSinglePosition_toOneVault_hack() public {
2805+
vm.startPrank(deployer);
2806+
2807+
_directDeposit(superformId1, 1e18);
2808+
2809+
ISuperformRouterPlus.RebalanceSinglePositionSyncArgs memory args =
2810+
_buildRebalanceSinglePositionToOneVaultArgsHack(deployer);
2811+
2812+
SuperPositions(SUPER_POSITIONS_SOURCE).increaseAllowance(ROUTER_PLUS_SOURCE, superformId1, args.sharesToRedeem);
2813+
vm.expectRevert(SuperformRouterPlus.RECEIVER_ADDRESS_MISMATCH.selector);
2814+
SuperformRouterPlus(ROUTER_PLUS_SOURCE).rebalanceSinglePosition{ value: 2 ether }(args);
2815+
}
2816+
27502817
/// @dev rebalance from a single position to a single vault
27512818
function test_rebalanceFromSinglePosition_toOneVault() public {
27522819
vm.startPrank(deployer);
@@ -5122,4 +5189,56 @@ contract SuperformRouterPlusTest is ProtocolActions {
51225189
SuperformRouterPlus(getContract(SOURCE_CHAIN, "SuperformRouterPlus")).setGlobalSlippage(100);
51235190
vm.stopPrank();
51245191
}
5192+
5193+
function test_deposit4626_maliciousReceiver() public {
5194+
// User approves router for vault tokens
5195+
vm.startPrank(deployer);
5196+
(address sfMigrateFrom,,) = superformId1.getSuperform();
5197+
address migrateFromVault = IBaseForm(sfMigrateFrom).getVaultAddress();
5198+
deal(migrateFromVault, deployer, 1e18);
5199+
5200+
// Approve and deposit DAI into the mock vault
5201+
MockERC20(getContract(SOURCE_CHAIN, "DAI")).approve(migrateFromVault, 1e18);
5202+
uint256 vaultTokenAmount = IERC4626(migrateFromVault).deposit(1e18, deployer);
5203+
IERC20(migrateFromVault).approve(getContract(SOURCE_CHAIN, "SuperformRouterPlus"), vaultTokenAmount);
5204+
5205+
vm.stopPrank();
5206+
5207+
address attacker = address(0x1337);
5208+
vm.deal(attacker, 1e18);
5209+
// Attacker prepares malicious deposit data
5210+
SingleVaultSFData memory data = SingleVaultSFData(
5211+
superformId1,
5212+
1e18,
5213+
1,
5214+
10_000,
5215+
LiqRequest("", address(0), address(0), 0, SOURCE_CHAIN, 0),
5216+
"",
5217+
false,
5218+
false,
5219+
attacker, // Set attacker as receiver
5220+
attacker, // Set attacker as receiverSP
5221+
""
5222+
);
5223+
5224+
bytes4 selector = IBaseRouter.singleDirectSingleVaultDeposit.selector;
5225+
bytes memory maliciousCallData = abi.encodePacked(selector, abi.encode(SingleDirectSingleVaultStateReq(data)));
5226+
5227+
address[] memory vaults = new address[](1);
5228+
vaults[0] = migrateFromVault;
5229+
5230+
ISuperformRouterPlus.Deposit4626Args[] memory args = new ISuperformRouterPlus.Deposit4626Args[](1);
5231+
args[0] = ISuperformRouterPlus.Deposit4626Args({
5232+
amount: 1e18,
5233+
expectedOutputAmount: 1,
5234+
maxSlippage: 9999,
5235+
receiverAddressSP: deployer,
5236+
depositCallData: maliciousCallData
5237+
});
5238+
5239+
// Attacker executes malicious deposit
5240+
vm.prank(attacker);
5241+
vm.expectRevert(SuperformRouterPlus.RECEIVER_ADDRESS_MISMATCH.selector);
5242+
SuperformRouterPlus(getContract(SOURCE_CHAIN, "SuperformRouterPlus")).deposit4626(vaults, args);
5243+
}
51255244
}

0 commit comments

Comments
 (0)