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

FbV2 sender nonces [SLT-183] #3214

Merged
merged 4 commits into from
Oct 2, 2024
Merged

FbV2 sender nonces [SLT-183] #3214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a913eb
sender nonces [SLT-183]
parodime Oct 2, 2024
15ca0de
refactor nonce test for codecov
parodime Oct 2, 2024
52da969
refactor: make `nonce` an immutable var
ChiTimesChi Oct 2, 2024
0cb26c4
chore: ignore local coverage report
ChiTimesChi Oct 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
4 changes: 3 additions & 1 deletion packages/contracts-rfq/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
flattened
.deployments
broadcast
broadcast

lcov.info
12 changes: 7 additions & 5 deletions packages/contracts-rfq/contracts/FastBridgeV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors {
mapping(bytes32 => BridgeTxDetails) public bridgeTxDetails;
/// @notice Relay details on destination chain
mapping(bytes32 => BridgeRelay) public bridgeRelayDetails;
/// @notice Unique bridge nonces tracked per originSender
mapping(address => uint256) public senderNonces;

/// @dev to prevent replays
uint256 public nonce;

// @dev the block the contract was deployed at
/// @notice This is deprecated and should not be used.
/// @dev Replaced by senderNonces
uint256 public immutable nonce = 0;
/// @notice the block the contract was deployed at
uint256 public immutable deployBlock;

constructor(address _owner) Admin(_owner) {
Expand Down Expand Up @@ -158,7 +160,7 @@ contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors {
originFeeAmount: originFeeAmount,
sendChainGas: params.sendChainGas,
deadline: params.deadline,
nonce: nonce++, // increment nonce on every bridge
nonce: senderNonces[params.sender]++, // increment nonce on every bridge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential off-by-one error due to postfix increment operator

The use of the postfix increment operator senderNonces[params.sender]++ assigns the current nonce value and then increments it. This means the nonce included in the request is the pre-incremented value. If the intention is to include the incremented nonce value, consider using the prefix increment operator ++senderNonces[params.sender] to avoid potential off-by-one errors.

Suggested change:

-                    nonce: senderNonces[params.sender]++, // increment nonce on every bridge
+                    nonce: ++senderNonces[params.sender], // increment nonce on every bridge
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
nonce: senderNonces[params.sender]++, // increment nonce on every bridge
nonce: ++senderNonces[params.sender], // increment nonce on every bridge

exclusivityRelayer: paramsV2.quoteRelayer,
// We checked exclusivityEndTime to be in range (0 .. params.deadline] above, so can safely cast
exclusivityEndTime: uint256(exclusivityEndTime)
Expand Down
6 changes: 6 additions & 0 deletions packages/contracts-rfq/test/FastBridgeV2.Src.Base.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ abstract contract FastBridgeV2SrcBaseTest is FastBridgeV2Test {
fastBridge.refund(abi.encode(bridgeTx));
}

function test_nonce() public view {
uint256 result = fastBridge.nonce();
// deprecated. should always return zero in FbV2.
assertEq(result, 0);
}

function assertEq(FastBridgeV2.BridgeStatus a, FastBridgeV2.BridgeStatus b) public pure {
assertEq(uint8(a), uint8(b));
}
Expand Down
18 changes: 17 additions & 1 deletion packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
expectBridgeRequested(tokenTx, txId);
expectBridgeQuoteDetails(txId, tokenParamsV2.quoteId);
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
checkTokenBalancesAfterBridge(userA);
}
Expand All @@ -112,6 +114,8 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
expectBridgeRequested(tokenTx, txId);
expectBridgeQuoteDetails(txId, tokenParamsV2.quoteId);
bridge({caller: userB, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
assertEq(srcToken.balanceOf(userA), LEFTOVER_BALANCE + tokenParams.originAmount);
checkTokenBalancesAfterBridge(userB);
Expand All @@ -126,29 +130,39 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
function test_bridge_eth() public {
// bridge token first to match the nonce
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
bytes32 txId = getTxId(ethTx);
expectBridgeRequested(ethTx, txId);
expectBridgeQuoteDetails(txId, ethParamsV2.quoteId);
bridge({caller: userA, msgValue: ethParams.originAmount, params: ethParams});
assertEq(fastBridge.senderNonces(userA), 2);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
checkEthBalancesAfterBridge(userA);
}

function test_bridge_eth_diffSender() public {
// bridge token first to match the nonce
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
bytes32 txId = getTxId(ethTx);
expectBridgeRequested(ethTx, txId);
expectBridgeQuoteDetails(txId, ethParamsV2.quoteId);
// bridge for user A as sender, called by userB
bridge({caller: userB, msgValue: ethParams.originAmount, params: ethParams});
assertEq(fastBridge.senderNonces(userA), 2);
assertEq(fastBridge.senderNonces(userB), 0);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
assertEq(userA.balance, LEFTOVER_BALANCE + ethParams.originAmount);
checkEthBalancesAfterBridge(userB);
}

function test_bridge_userSpecificNonce() public {
vm.skip(true); // TODO: unskip when implemented
bridge({caller: userA, msgValue: 0, params: tokenParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 0);
// UserB nonce is 0
ethTx.nonce = 0;
ethParams.sender = userB;
Expand All @@ -157,6 +171,8 @@ contract FastBridgeV2SrcTest is FastBridgeV2SrcBaseTest {
expectBridgeRequested(ethTx, txId);
expectBridgeQuoteDetails(txId, ethParamsV2.quoteId);
bridge({caller: userB, msgValue: ethParams.originAmount, params: ethParams});
assertEq(fastBridge.senderNonces(userA), 1);
assertEq(fastBridge.senderNonces(userB), 1);
assertEq(fastBridge.bridgeStatuses(txId), IFastBridgeV2.BridgeStatus.REQUESTED);
checkEthBalancesAfterBridge(userB);
}
Expand Down
Loading