Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove revert cases in exec race conditions #1246

Merged
merged 7 commits into from
Aug 2, 2024
Merged

Remove revert cases in exec race conditions #1246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
395cbdb
normal offramp
RensR Aug 1, 2024
45351f2
multi offramp
RensR Aug 1, 2024
602eacf
Merge 45351f25a6f1ef2dedc67b7a72f663454a73d07d into a2566c07ea9d09dbd…
RensR Aug 1, 2024
e9c8e7f
Update gethwrappers
app-token-issuer-infra-releng[bot] Aug 1, 2024
a4c3e06
update interface
RensR Aug 1, 2024
8f9b37f
Merge branch 'ccip-develop' into remove-reverts-if-msg-is-attempted
RensR Aug 2, 2024
557c89b
Merge branch 'ccip-develop' into remove-reverts-if-msg-is-attempted
RensR Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
202 changes: 101 additions & 101 deletions contracts/gas-snapshots/ccip.gas-snapshot
View file
Open in desktop

Large diffs are not rendered by default.

20 changes: 10 additions & 10 deletions contracts/src/v0.8/ccip/offRamp/EVM2EVMMultiOffRamp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
using ERC165Checker for address;
using EnumerableMapAddresses for EnumerableMapAddresses.AddressToAddressMap;

error AlreadyAttempted(uint64 sourceChainSelector, uint64 sequenceNumber);
error AlreadyExecuted(uint64 sourceChainSelector, uint64 sequenceNumber);
error ZeroChainSelectorNotAllowed();
error ExecutionError(bytes32 messageId, bytes err);
Expand Down Expand Up @@ -74,6 +73,7 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
event SourceChainSelectorAdded(uint64 sourceChainSelector);
event SourceChainConfigSet(uint64 indexed sourceChainSelector, SourceChainConfig sourceConfig);
event SkippedAlreadyExecutedMessage(uint64 sourceChainSelector, uint64 sequenceNumber);
event AlreadyAttempted(uint64 sourceChainSelector, uint64 sequenceNumber);
/// @dev RMN depends on this event, if changing, please notify the RMN maintainers.
event CommitReportAccepted(CommitReport report);
event RootRemoved(bytes32 root);
Expand Down Expand Up @@ -375,13 +375,6 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {

Internal.MessageExecutionState originalState =
getExecutionState(sourceChainSelector, message.header.sequenceNumber);
if (originalState == Internal.MessageExecutionState.SUCCESS) {
// If the message has already been executed, we skip it. We want to not revert on race conditions between
// executing parties. This will allow us to open up manual exec while also attempting with the DON, without
// reverting an entire DON batch when a user manually executes while the tx is inflight.
emit SkippedAlreadyExecutedMessage(sourceChainSelector, message.header.sequenceNumber);
continue;
}
// Two valid cases here, we either have never touched this message before, or we tried to execute
// and failed. This check protects against reentry and re-execution because the other state is
// IN_PROGRESS which should not be allowed to execute.
Expand All @@ -390,7 +383,13 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
originalState == Internal.MessageExecutionState.UNTOUCHED
|| originalState == Internal.MessageExecutionState.FAILURE
)
) revert AlreadyExecuted(sourceChainSelector, message.header.sequenceNumber);
) {
// If the message has already been executed, we skip it. We want to not revert on race conditions between
// executing parties. This will allow us to open up manual exec while also attempting with the DON, without
// reverting an entire DON batch when a user manually executes while the tx is inflight.
emit SkippedAlreadyExecutedMessage(sourceChainSelector, message.header.sequenceNumber);
continue;
}

if (manualExecution) {
bool isOldCommitReport =
Expand All @@ -409,7 +408,8 @@ contract EVM2EVMMultiOffRamp is ITypeAndVersion, MultiOCR3Base {
// DON can only execute a message once
// Acceptable state transitions: UNTOUCHED->SUCCESS, UNTOUCHED->FAILURE
if (originalState != Internal.MessageExecutionState.UNTOUCHED) {
revert AlreadyAttempted(sourceChainSelector, message.header.sequenceNumber);
emit AlreadyAttempted(sourceChainSelector, message.header.sequenceNumber);
continue;
}
}

Expand Down
22 changes: 12 additions & 10 deletions contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
using ERC165Checker for address;
using EnumerableMapAddresses for EnumerableMapAddresses.AddressToAddressMap;

error AlreadyAttempted(uint64 sequenceNumber);
error AlreadyExecuted(uint64 sequenceNumber);
error ZeroAddressNotAllowed();
error CommitStoreAlreadyInUse();
Expand Down Expand Up @@ -69,6 +68,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
event TokenAggregateRateLimitAdded(address sourceToken, address destToken);
event TokenAggregateRateLimitRemoved(address sourceToken, address destToken);
event SkippedAlreadyExecutedMessage(uint64 indexed sequenceNumber);
event AlreadyAttempted(uint64 sequenceNumber);

/// @notice Static offRamp config
/// @dev RMN depends on this struct, if changing, please notify the RMN maintainers.
Expand Down Expand Up @@ -276,13 +276,6 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
for (uint256 i = 0; i < numMsgs; ++i) {
Internal.EVM2EVMMessage memory message = report.messages[i];
Internal.MessageExecutionState originalState = getExecutionState(message.sequenceNumber);
if (originalState == Internal.MessageExecutionState.SUCCESS) {
// If the message has already been executed, we skip it. We want to not revert on race conditions between
// executing parties. This will allow us to open up manual exec while also attempting with the DON, without
// reverting an entire DON batch when a user manually executes while the tx is inflight.
emit SkippedAlreadyExecutedMessage(message.sequenceNumber);
continue;
}
// Two valid cases here, we either have never touched this message before, or we tried to execute
// and failed. This check protects against reentry and re-execution because the other state is
// IN_PROGRESS which should not be allowed to execute.
Expand All @@ -291,7 +284,13 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
originalState == Internal.MessageExecutionState.UNTOUCHED
|| originalState == Internal.MessageExecutionState.FAILURE
)
) revert AlreadyExecuted(message.sequenceNumber);
) {
// If the message has already been executed, we skip it. We want to not revert on race conditions between
// executing parties. This will allow us to open up manual exec while also attempting with the DON, without
// reverting an entire DON batch when a user manually executes while the tx is inflight.
emit SkippedAlreadyExecutedMessage(message.sequenceNumber);
continue;
}

if (manualExecution) {
bool isOldCommitReport =
Expand All @@ -309,7 +308,10 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
} else {
// DON can only execute a message once
// Acceptable state transitions: UNTOUCHED->SUCCESS, UNTOUCHED->FAILURE
if (originalState != Internal.MessageExecutionState.UNTOUCHED) revert AlreadyAttempted(message.sequenceNumber);
if (originalState != Internal.MessageExecutionState.UNTOUCHED) {
emit AlreadyAttempted(message.sequenceNumber);
continue;
}
}

if (message.nonce != 0) {
Expand Down
34 changes: 15 additions & 19 deletions contracts/src/v0.8/ccip/test/offRamp/EVM2EVMMultiOffRamp.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,11 +945,10 @@ contract EVM2EVMMultiOffRamp_executeSingleReport is EVM2EVMMultiOffRampSetup {
);
s_offRamp.executeSingleReport(_generateReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), new uint256[](0));

vm.expectRevert(
abi.encodeWithSelector(
EVM2EVMMultiOffRamp.AlreadyAttempted.selector, SOURCE_CHAIN_SELECTOR_1, messages[0].header.sequenceNumber
)
);
// The second time should skip the msg
vm.expectEmit();
emit EVM2EVMMultiOffRamp.AlreadyAttempted(SOURCE_CHAIN_SELECTOR_1, messages[0].header.sequenceNumber);

s_offRamp.executeSingleReport(_generateReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), new uint256[](0));
}

Expand Down Expand Up @@ -1681,7 +1680,7 @@ contract EVM2EVMMultiOffRamp_manuallyExecute is EVM2EVMMultiOffRampSetup {
s_offRamp.manuallyExecute(_generateBatchReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), gasLimitOverrides);
}

function test_manuallyExecute_ReentrancyFails() public {
function test_manuallyExecute_ReentrancyFails_Success() public {
uint256 tokenAmount = 1e9;
IERC20 tokenToAbuse = IERC20(s_destFeeToken);

Expand Down Expand Up @@ -1715,29 +1714,26 @@ contract EVM2EVMMultiOffRamp_manuallyExecute is EVM2EVMMultiOffRampSetup {
uint256[][] memory gasLimitOverrides = new uint256[][](1);
gasLimitOverrides[0] = _getGasLimitsFromMessages(messages);

// The first entry should be fine and triggers the second entry. This one fails
// but since it's an inner tx of the first one it is caught in the try-catch.
// This means the first tx is marked `FAILURE` with the error message of the second tx.
// The first entry should be fine and triggers the second entry which is skipped. Due to the reentrancy
// the second completes first, so we expect the skip event before the success event.
vm.expectEmit();
emit EVM2EVMMultiOffRamp.SkippedAlreadyExecutedMessage(
messages[0].header.sourceChainSelector, messages[0].header.sequenceNumber
);

vm.expectEmit();
emit EVM2EVMMultiOffRamp.ExecutionStateChanged(
messages[0].header.sourceChainSelector,
messages[0].header.sequenceNumber,
messages[0].header.messageId,
Internal.MessageExecutionState.FAILURE,
abi.encodeWithSelector(
EVM2EVMMultiOffRamp.ReceiverError.selector,
abi.encodeWithSelector(
EVM2EVMMultiOffRamp.AlreadyExecuted.selector,
messages[0].header.sourceChainSelector,
messages[0].header.sequenceNumber
)
)
Internal.MessageExecutionState.SUCCESS,
""
);

s_offRamp.manuallyExecute(_generateBatchReportFromMessages(SOURCE_CHAIN_SELECTOR_1, messages), gasLimitOverrides);

// Since the tx failed we don't release the tokens
assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre);
assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre + tokenAmount);
}
}

Expand Down
159 changes: 79 additions & 80 deletions contracts/src/v0.8/ccip/test/offRamp/EVM2EVMOffRamp.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,36 @@ contract EVM2EVMOffRamp_execute is EVM2EVMOffRampSetup {
s_offRamp.execute(executionReport, new uint256[](0));
}

function test_RetryFailedMessageWithoutManualExecution_Success() public {
Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();

bytes memory realError1 = new bytes(2);
realError1[0] = 0xbe;
realError1[1] = 0xef;
s_reverting_receiver.setErr(realError1);

messages[0].receiver = address(s_reverting_receiver);
messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());

vm.expectEmit();
emit EVM2EVMOffRamp.ExecutionStateChanged(
messages[0].sequenceNumber,
messages[0].messageId,
Internal.MessageExecutionState.FAILURE,
abi.encodeWithSelector(
EVM2EVMOffRamp.ReceiverError.selector,
abi.encodeWithSelector(MaybeRevertMessageReceiver.CustomError.selector, realError1)
)
);
s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));

// The second time should skip the msg
vm.expectEmit();
emit EVM2EVMOffRamp.AlreadyAttempted(messages[0].sequenceNumber);

s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));
}

// Reverts

function test_InvalidMessageId_Revert() public {
Expand Down Expand Up @@ -744,33 +774,6 @@ contract EVM2EVMOffRamp_execute is EVM2EVMOffRampSetup {
);
s_offRamp.execute(executionReport, new uint256[](0));
}

function test_RetryFailedMessageWithoutManualExecution_Revert() public {
Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();

bytes memory realError1 = new bytes(2);
realError1[0] = 0xbe;
realError1[1] = 0xef;
s_reverting_receiver.setErr(realError1);

messages[0].receiver = address(s_reverting_receiver);
messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());

vm.expectEmit();
emit EVM2EVMOffRamp.ExecutionStateChanged(
messages[0].sequenceNumber,
messages[0].messageId,
Internal.MessageExecutionState.FAILURE,
abi.encodeWithSelector(
EVM2EVMOffRamp.ReceiverError.selector,
abi.encodeWithSelector(MaybeRevertMessageReceiver.CustomError.selector, realError1)
)
);
s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));

vm.expectRevert(abi.encodeWithSelector(EVM2EVMOffRamp.AlreadyAttempted.selector, messages[0].sequenceNumber));
s_offRamp.execute(_generateReportFromMessages(messages), new uint256[](0));
}
}

contract EVM2EVMOffRamp_execute_upgrade is EVM2EVMOffRampSetup {
Expand Down Expand Up @@ -1144,6 +1147,55 @@ contract EVM2EVMOffRamp_manuallyExecute is EVM2EVMOffRampSetup {
s_offRamp.manuallyExecute(_generateReportFromMessages(messages), gasLimitOverrides);
}

function test_ReentrancyManualExecuteFails_Success() public {
uint256 tokenAmount = 1e9;
IERC20 tokenToAbuse = IERC20(s_destFeeToken);

// This needs to be deployed before the source chain message is sent
// because we need the address for the receiver.
ReentrancyAbuser receiver = new ReentrancyAbuser(address(s_destRouter), s_offRamp);
uint256 balancePre = tokenToAbuse.balanceOf(address(receiver));

// For this test any message will be flagged as correct by the
// commitStore. In a real scenario the abuser would have to actually
// send the message that they want to replay.
Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();
messages[0].tokenAmounts = new Client.EVMTokenAmount[](1);
messages[0].tokenAmounts[0] = Client.EVMTokenAmount({token: s_sourceFeeToken, amount: tokenAmount});
messages[0].receiver = address(receiver);
messages[0].sourceTokenData = new bytes[](1);
messages[0].sourceTokenData[0] = abi.encode(
Internal.SourceTokenData({
sourcePoolAddress: abi.encode(s_sourcePoolByToken[s_sourceFeeToken]),
destTokenAddress: abi.encode(s_destTokenBySourceToken[s_sourceFeeToken]),
extraData: "",
destGasAmount: DEFAULT_TOKEN_DEST_GAS_OVERHEAD
})
);

messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());

Internal.ExecutionReport memory report = _generateReportFromMessages(messages);

// sets the report to be repeated on the ReentrancyAbuser to be able to replay
receiver.setPayload(report);

// The first entry should be fine and triggers the second entry which is skipped. Due to the reentrancy
// the second completes first, so we expect the skip event before the success event.
vm.expectEmit();
emit EVM2EVMOffRamp.SkippedAlreadyExecutedMessage(messages[0].sequenceNumber);

vm.expectEmit();
emit EVM2EVMOffRamp.ExecutionStateChanged(
messages[0].sequenceNumber, messages[0].messageId, Internal.MessageExecutionState.SUCCESS, ""
);

s_offRamp.manuallyExecute(report, _getGasLimitsFromMessages(messages));

// Assert that they only got the tokens once, not twice
assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre + tokenAmount);
}

function test_ManualExecForkedChain_Revert() public {
Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();

Expand Down Expand Up @@ -1200,59 +1252,6 @@ contract EVM2EVMOffRamp_manuallyExecute is EVM2EVMOffRampSetup {
);
s_offRamp.manuallyExecute(_generateReportFromMessages(messages), _getGasLimitsFromMessages(messages));
}

function test_ReentrancyManualExecuteFails() public {
uint256 tokenAmount = 1e9;
IERC20 tokenToAbuse = IERC20(s_destFeeToken);

// This needs to be deployed before the source chain message is sent
// because we need the address for the receiver.
ReentrancyAbuser receiver = new ReentrancyAbuser(address(s_destRouter), s_offRamp);
uint256 balancePre = tokenToAbuse.balanceOf(address(receiver));

// For this test any message will be flagged as correct by the
// commitStore. In a real scenario the abuser would have to actually
// send the message that they want to replay.
Internal.EVM2EVMMessage[] memory messages = _generateSingleBasicMessage();
messages[0].tokenAmounts = new Client.EVMTokenAmount[](1);
messages[0].tokenAmounts[0] = Client.EVMTokenAmount({token: s_sourceFeeToken, amount: tokenAmount});
messages[0].receiver = address(receiver);
messages[0].sourceTokenData = new bytes[](1);
messages[0].sourceTokenData[0] = abi.encode(
Internal.SourceTokenData({
sourcePoolAddress: abi.encode(s_sourcePoolByToken[s_sourceFeeToken]),
destTokenAddress: abi.encode(s_destTokenBySourceToken[s_sourceFeeToken]),
extraData: "",
destGasAmount: DEFAULT_TOKEN_DEST_GAS_OVERHEAD
})
);

messages[0].messageId = Internal._hash(messages[0], s_offRamp.metadataHash());

Internal.ExecutionReport memory report = _generateReportFromMessages(messages);

// sets the report to be repeated on the ReentrancyAbuser to be able to replay
receiver.setPayload(report);

// The first entry should be fine and triggers the second entry. This one fails
// but since it's an inner tx of the first one it is caught in the try-catch.
// This means the first tx is marked `FAILURE` with the error message of the second tx.
vm.expectEmit();
emit EVM2EVMOffRamp.ExecutionStateChanged(
messages[0].sequenceNumber,
messages[0].messageId,
Internal.MessageExecutionState.FAILURE,
abi.encodeWithSelector(
EVM2EVMOffRamp.ReceiverError.selector,
abi.encodeWithSelector(EVM2EVMOffRamp.AlreadyExecuted.selector, messages[0].sequenceNumber)
)
);

s_offRamp.manuallyExecute(report, _getGasLimitsFromMessages(messages));

// Since the tx failed we don't release the tokens
assertEq(tokenToAbuse.balanceOf(address(receiver)), balancePre);
}
}

contract EVM2EVMOffRamp_getExecutionState is EVM2EVMOffRampSetup {
Expand Down
134 changes: 132 additions & 2 deletions core/gethwrappers/ccip/generated/evm_2_evm_multi_offramp/evm_2_evm_multi_offramp.go
View file
Open in desktop

Large diffs are not rendered by default.

133 changes: 131 additions & 2 deletions core/gethwrappers/ccip/generated/evm_2_evm_offramp/evm_2_evm_offramp.go
View file
Open in desktop

Large diffs are not rendered by default.

Loading
Loading