Skip to content

Commit

Permalink
Merge pull request #639 from superform-xyz/tamara-sup-8873-guard-agai…
Browse files Browse the repository at this point in the history 
…nst-malicious-processors

fix: Guard against malicious processors [SUP-8873]
  • Loading branch information
@0xTimepunk
0xTimepunk authored Oct 25, 2024
2 parents 0a7a58b + 48e3015 commit 3d85405
Show file tree
Hide file tree
Showing 7 changed files with 120 additions and 108 deletions.
2 changes: 1 addition & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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-contract SDMVW0TokenInputNoSlippageAMB1323 --evm-version cancun -vvv
forge test --match-test test_crossChainRebalance_updateSuperformData_allErrors --evm-version cancun -vvv

.PHONY: ftest
ftest: ## Runs tests with cancun evm version
Expand Down
45 changes: 26 additions & 19 deletions src/interfaces/ISuperformRouterPlusAsync.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
/// @notice thrown if the caller is not router plus
error NOT_ROUTER_PLUS();

/// @notice thrown if the caller is not core state registry rescuer
error NOT_CORE_STATE_REGISTRY_RESCUER();

/// @notice thrown if the rebalance to update is invalid
error COMPLETE_REBALANCE_INVALID_TX_DATA_UPDATE();

Expand All @@ -44,17 +47,17 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
/// @notice thrown to avoid processing the same rebalance payload twice
error REBALANCE_ALREADY_PROCESSED();

/// @notice thrown when the refund proposer is invalid
error INVALID_PROPOSER();
/// @notice thrown when the refund requester is not the payload receiver
error INVALID_REQUESTER();

/// @notice thrown when the refund payload is invalid
error INVALID_REFUND_DATA();

/// @notice thrown when refund is already proposed
error REFUND_ALREADY_PROPOSED();
/// @notice thrown when requested refund amount is too high
error REQUESTED_AMOUNT_TOO_HIGH();

/// @notice thrown if the refund is still in dispute phase
error IN_DISPUTE_PHASE();
/// @notice thrown when the refund payload is already approved
error REFUND_ALREADY_APPROVED();

//////////////////////////////////////////////////////////////
// EVENTS //
Expand All @@ -74,6 +77,15 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
uint256 indexed routerPlusPayloadId, address indexed refundReceiver, address refundToken, uint256 refundAmount
);

/// @notice emitted when a refund is proposed
/// @param routerPlusPayloadId is the unique identifier for the payload
/// @param refundReceiver is the address of the user who'll receiver the refund
/// @param refundToken is the token to be refunded
/// @param refundAmount is the new refund amount
event refundRequested(
uint256 indexed routerPlusPayloadId, address indexed refundReceiver, address refundToken, uint256 refundAmount
);

/// @notice emitted when an existing refund got disputed
/// @param routerPlusPayloadId is the unique identifier for the payload
/// @param disputer is the address of the user who disputed the refund
Expand All @@ -97,7 +109,6 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
address receiver;
address interimToken;
uint256 amount;
uint256 proposedTime;
}

struct DecodedRouterPlusRebalanceCallData {
Expand Down Expand Up @@ -168,16 +179,12 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
payable
returns (bool rebalanceSuccessful);

/// @notice allows the receiver / disputer to protect against malicious processors
/// @param finalPayloadId_ is the unique identifier of the refund
function disputeRefund(uint256 finalPayloadId_) external;

/// @notice allows the rescuer to propose a new refund amount after a successful dispute
/// @param finalPayloadId_ is the unique identifier of the refund
/// @param refundAmount_ is the new refund amount proposed
function proposeRefund(uint256 finalPayloadId_, uint256 refundAmount_) external;
/// @notice allows the user to request a refund for the rebalance
/// @param routerplusPayloadId_ the router plus payload id
function requestRefund(uint256 routerplusPayloadId_, uint256 requestedAmount) external;

/// @notice allows the user to claim their refund post the dispute period
/// @param finalPayloadId_ is the unique identifier of the refund
function finalizeRefund(uint256 finalPayloadId_) external;
}
/// @dev only callable by core state registry rescuer
/// @notice approves a refund for the rebalance and sends funds to the receiver
/// @param routerplusPayloadId_ the router plus payload id
function approveRefund(uint256 routerplusPayloadId_) external;
}
63 changes: 26 additions & 37 deletions src/router-plus/SuperformRouterPlusAsync.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou

mapping(uint256 routerPlusPayloadId => Refund) public refunds;
mapping(uint256 routerPlusPayloadId => bool processed) public processedRebalancePayload;

mapping(uint256 routerPlusPayloadId => bool approvedRefund) public approvedRefund;

//////////////////////////////////////////////////////////////
// MODIFIERS //
//////////////////////////////////////////////////////////////
Expand All @@ -52,6 +55,13 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
_;
}

modifier onlyCoreStateRegistryRescuer() {
if (!_hasRole(keccak256("CORE_STATE_REGISTRY_RESCUER_ROLE"), msg.sender)) {
revert NOT_CORE_STATE_REGISTRY_RESCUER();
}
_;
}

//////////////////////////////////////////////////////////////
// CONSTRUCTOR //
//////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -252,8 +262,8 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
ENTIRE_SLIPPAGE * args_.amountReceivedInterimAsset
< ((data.expectedAmountInterimAsset * (ENTIRE_SLIPPAGE - data.slippage)))
) {
refunds[args_.routerPlusPayloadId] =
Refund(args_.receiverAddressSP, data.interimAsset, args_.amountReceivedInterimAsset, block.timestamp);

refunds[args_.routerPlusPayloadId] = Refund(args_.receiverAddressSP, data.interimAsset, 0);

emit RefundInitiated(
args_.routerPlusPayloadId, args_.receiverAddressSP, data.interimAsset, args_.amountReceivedInterimAsset
Expand Down Expand Up @@ -416,42 +426,30 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
}

/// @inheritdoc ISuperformRouterPlusAsync
function disputeRefund(uint256 routerPlusPayloadId_) external override {
Refund storage r = refunds[routerPlusPayloadId_];

if (!(msg.sender == r.receiver || _hasRole(keccak256("CORE_STATE_REGISTRY_DISPUTER_ROLE"), msg.sender))) {
revert Error.NOT_VALID_DISPUTER();
}

if (r.proposedTime == 0 || block.timestamp > r.proposedTime + _getDelay()) revert Error.DISPUTE_TIME_ELAPSED();

/// @dev just can reset the last proposed time, since amounts should be updated again to
/// pass the proposedTime zero check in finalize
r.proposedTime = 0;

emit RefundDisputed(routerPlusPayloadId_, msg.sender);
}
function requestRefund(uint256 routerPlusPayloadId_, uint256 requestedAmount) external {
Refund memory r = refunds[routerPlusPayloadId_];

/// @inheritdoc ISuperformRouterPlusAsync
function proposeRefund(uint256 routerPlusPayloadId_, uint256 refundAmount_) external {
if (!_hasRole(keccak256("CORE_STATE_REGISTRY_RESCUER_ROLE"), msg.sender)) revert INVALID_PROPOSER();
if (msg.sender != r.receiver) revert INVALID_REQUESTER();
if (r.interimToken == address(0)) revert INVALID_REFUND_DATA();

Refund storage r = refunds[routerPlusPayloadId_];
XChainRebalanceData memory data = xChainRebalanceCallData[r.receiver][routerPlusPayloadId_];

if (r.interimToken == address(0) || r.receiver == address(0)) revert INVALID_REFUND_DATA();
if (r.proposedTime != 0) revert REFUND_ALREADY_PROPOSED();
if (requestedAmount > data.expectedAmountInterimAsset) {
revert REQUESTED_AMOUNT_TOO_HIGH();
}

r.proposedTime = block.timestamp;
r.amount = refundAmount_;
refunds[routerPlusPayloadId_] = Refund(msg.sender, data.interimAsset, requestedAmount);

emit NewRefundAmountProposed(routerPlusPayloadId_, refundAmount_);
emit refundRequested(routerPlusPayloadId_, msg.sender, r.interimToken, requestedAmount);
}

/// @inheritdoc ISuperformRouterPlusAsync
function finalizeRefund(uint256 routerPlusPayloadId_) external {
function approveRefund(uint256 routerPlusPayloadId_) external onlyCoreStateRegistryRescuer {
if (approvedRefund[routerPlusPayloadId_]) revert REFUND_ALREADY_APPROVED();

Refund memory r = refunds[routerPlusPayloadId_];

if (r.proposedTime == 0 || block.timestamp <= r.proposedTime + _getDelay()) revert IN_DISPUTE_PHASE();
approvedRefund[routerPlusPayloadId_] = true;

/// @dev deleting to prevent re-entrancy
delete refunds[routerPlusPayloadId_];
Expand All @@ -465,15 +463,6 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
// INTERNAL FUNCTIONS //
//////////////////////////////////////////////////////////////

/// @dev returns the current dispute delay
function _getDelay() internal view returns (uint256) {
uint256 delay = superRegistry.delay();
if (delay == 0) {
revert Error.DELAY_NOT_SET();
}
return delay;
}

function _updateSuperformData(
MultiVaultSFData memory sfData,
LiqRequest[] memory liqRequests,
Expand Down
14 changes: 8 additions & 6 deletions test/fuzz/crosschain-data/adapters/HyperlaneImplementation.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,21 +107,23 @@ contract HyperlaneImplementationTest is CommonProtocolActions {
vm.startPrank(deployer);
uint256 userIndex = userSeed_ % users.length;


vm.assume(malice_ != getContract(ETH, "CoreStateRegistry"));
vm.assume(malice_ != getContract(ETH, "TimelockStateRegistry"));
vm.assume(malice_ != getContract(ETH, "BroadcastRegistry"));
vm.assume(malice_ != getContract(ETH, "AsyncStateRegistry"));

AMBMessage memory ambMessage;
BroadCastAMBExtraData memory ambExtraData;
address coreStateRegistry;

(ambMessage, ambExtraData, coreStateRegistry) =
setupBroadcastPayloadAMBData(users[userIndex], address(hyperlaneImplementation));

vm.expectRevert(Error.NOT_STATE_REGISTRY.selector);
vm.assume(malice_ != getContract(ETH, "CoreStateRegistry"));
vm.assume(malice_ != getContract(ETH, "TimelockStateRegistry"));
vm.assume(malice_ != getContract(ETH, "BroadcastRegistry"));
vm.assume(malice_ != getContract(ETH, "AsyncStateRegistry"));
vm.stopPrank();

vm.deal(malice_, 100 ether);
vm.prank(malice_);
vm.expectRevert(Error.NOT_STATE_REGISTRY.selector);
hyperlaneImplementation.dispatchPayload{ value: 0.1 ether }(
users[userIndex], chainIds[5], abi.encode(ambMessage), abi.encode(ambExtraData)
);
Expand Down
3 changes: 1 addition & 2 deletions test/mainnet/SmokeTest.Staging.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,14 +555,13 @@ contract SmokeTestStaging is MainnetBaseSetup {

for (uint256 i; i < TARGET_DEPLOYMENT_CHAINS.length; ++i) {
uint64 chainId = TARGET_DEPLOYMENT_CHAINS[i];

vm.selectFork(FORKS[chainId]);
superFactory = SuperformFactory(getContract(chainId, "SuperformFactory"));

chainId != BLAST
? assertEq(superFactory.getFormImplementation(5), getContract(chainId, "ERC5115Form"))
: assertEq(superFactory.getFormImplementation(205), getContract(chainId, "ERC5115Form"));
/// @dev in blast there are 6 forms (2 without operator, 2 with wrong state registry and 2 right superforms)

assertEq(superFactory.getFormCount(), chainId == LINEA ? 5 : chainId == BLAST ? 8 : 7);
assertEq(superFactory.getFormStateRegistryId(5), 1);
}
Expand Down
1 change: 0 additions & 1 deletion test/unit/payments/PaymentHelper.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,6 @@ contract PaymentHelperTest is ProtocolActions {

assertEq(fees3, 0);
}


function test_estimateSingleDirectMultiVault() public view {
/// @dev scenario: single vault withdrawal
Expand Down
100 changes: 58 additions & 42 deletions test/unit/router-plus/SuperformRouterPlus.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1829,56 +1829,65 @@ contract SuperformRouterPlusTest is ProtocolActions {
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether }(completeArgs);
vm.stopPrank();

vm.expectRevert(Error.NOT_VALID_DISPUTER.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
// Step 5: Request refund

vm.startPrank(deployer);
vm.mockCall(
address(getContract(SOURCE_CHAIN, "SuperRegistry")),
abi.encodeWithSelector(ISuperRegistry.delay.selector),
abi.encode(0)
);
vm.expectRevert(Error.DELAY_NOT_SET.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
vm.clearMockedCalls();

vm.warp(block.timestamp + 100 days);
vm.expectRevert(Error.DISPUTE_TIME_ELAPSED.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);

vm.warp(block.timestamp - 100 days);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
/// @dev testing invalid requester (not receiver)
vm.startPrank(address(222));
vm.expectRevert(ISuperformRouterPlusAsync.INVALID_REQUESTER.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).requestRefund(1, 100);
vm.stopPrank();

vm.expectRevert(Error.DISPUTE_TIME_ELAPSED.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
// @dev testing refund amount exceeds expected amount
vm.startPrank(deployer);
vm.expectRevert(ISuperformRouterPlusAsync.REQUESTED_AMOUNT_TOO_HIGH.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).requestRefund(1, 1000e18);
vm.stopPrank();

vm.expectRevert(ISuperformRouterPlusAsync.INVALID_PROPOSER.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(1, completeArgs.amountReceivedInterimAsset);
/// @dev testing valid refund request
vm.prank(deployer);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).requestRefund(1, 100);

vm.startPrank(deployer);
vm.expectRevert(ISuperformRouterPlusAsync.INVALID_REFUND_DATA.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(2, completeArgs.amountReceivedInterimAsset);
(,, uint256 requestedAmount) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
assertEq(requestedAmount, 100);

SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(1, completeArgs.amountReceivedInterimAsset);
(, address refundToken,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
assertEq(refundToken, address(args.interimAsset));

vm.expectRevert(ISuperformRouterPlusAsync.REFUND_ALREADY_PROPOSED.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(1, completeArgs.amountReceivedInterimAsset);
// Step 6: Approve refund

vm.expectRevert(ISuperformRouterPlusAsync.IN_DISPUTE_PHASE.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).finalizeRefund(1);
/// @dev testing invalid approver (not core state registry)
vm.startPrank(address(1234));
vm.expectRevert(ISuperformRouterPlusAsync.NOT_CORE_STATE_REGISTRY_RESCUER.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).approveRefund(1);
vm.stopPrank();

(, address refundToken,,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
/// @dev testing valid refund approval
uint256 balanceBefore = MockERC20(refundToken).balanceOf(deployer);
uint256 routerBalanceBefore = MockERC20(refundToken).balanceOf(address(ROUTER_PLUS_ASYNC_SOURCE));
vm.startPrank(deployer);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).approveRefund(1);
vm.stopPrank();

vm.warp(block.timestamp + 100 days);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).finalizeRefund(1);
uint256 balanceAfter = MockERC20(refundToken).balanceOf(deployer);

assertGt(balanceAfter, balanceBefore);
assertEq(MockERC20(refundToken).balanceOf(address(ROUTER_PLUS_ASYNC_SOURCE)), routerBalanceBefore - 100);
assertEq(MockERC20(refundToken).balanceOf(address(deployer)), balanceBefore + 100);

(, address interimToken,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
assertEq(interimToken, address(0));

(, address receiver,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
assertEq(receiver, address(0));

vm.expectRevert(ISuperformRouterPlusAsync.IN_DISPUTE_PHASE.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).finalizeRefund(1);
(,, uint256 updatedRequestedAmount) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
assertEq(updatedRequestedAmount, 0);
vm.stopPrank();

/// @dev testing refund already approved
vm.startPrank(deployer);
vm.expectRevert(ISuperformRouterPlusAsync.REFUND_ALREADY_APPROVED.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).approveRefund(1);
vm.stopPrank();
}

function test_crossChainRebalance_negativeSlippage() public {
Expand Down Expand Up @@ -1935,6 +1944,7 @@ contract SuperformRouterPlusTest is ProtocolActions {

function test_crossChainRebalance_updateSuperformData_allErrors() public {
vm.selectFork(FORKS[SOURCE_CHAIN]);

SingleVaultSFData memory sfData = SingleVaultSFData({
superformId: superformId1,
amount: 1e18,
Expand Down Expand Up @@ -2042,14 +2052,19 @@ contract SuperformRouterPlusTest is ProtocolActions {

// Reset liqDstChainId
completeArgs.liqRequests[0][0].liqDstChainId = SOURCE_CHAIN;

/// @FIXME: This line is not reacheable
// vm.expectRevert(ISuperformRouterPlusAsync.COMPLETE_REBALANCE_DIFFERENT_RECEIVER.selector);
// completeArgs.receiverAddressSP = address(0x123);
// SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether
// }(completeArgs);
vm.stopPrank();
vm.startPrank(ROUTER_PLUS_SOURCE);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).setXChainRebalanceCallData(address(0x123), 2, data);
vm.stopPrank();
vm.startPrank(deployer);

vm.expectRevert(ISuperformRouterPlusAsync.COMPLETE_REBALANCE_DIFFERENT_RECEIVER.selector);
completeArgs.routerPlusPayloadId = 2;
completeArgs.receiverAddressSP = address(0x123);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether }(completeArgs);
vm.stopPrank();
completeArgs.routerPlusPayloadId = 1;
completeArgs.receiverAddressSP = deployer;
sfData.liqRequest.token = address(0);

data = IBaseSuperformRouterPlus.XChainRebalanceData({
Expand All @@ -2070,6 +2085,7 @@ contract SuperformRouterPlusTest is ProtocolActions {
completeArgs.routerPlusPayloadId = 2;
vm.expectRevert(ISuperformRouterPlusAsync.COMPLETE_REBALANCE_INVALID_TX_DATA_UPDATE.selector);
SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether }(completeArgs);
vm.stopPrank();
}

//////////////////////////////////////////////////////////////
Expand Down

0 comments on commit 3d85405

Please sign in to comment.