Skip to content

Commit

Permalink
Use n=4 for solhint func-params rule (#279)
Browse files Browse the repository at this point in the history
  • Loading branch information
dnkolegov authored Mar 24, 2024
1 parent 90fa37f commit a52ddf6
Show file tree
Hide file tree
Showing 35 changed files with 287 additions and 96 deletions.
2 changes: 1 addition & 1 deletion .solhint.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"use-forbidden-name": "error",
"visibility-modifier-order": "error",
"reentrancy": "error",
"func-named-parameters": ["error", 5],
"func-named-parameters": ["error", 4],
"compiler-version": ["error", "^0.8.0"]
}
}
15 changes: 8 additions & 7 deletions l1-contracts/contracts/bridge/L1ERC20Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
_refundRecipient: _refundRecipient
});
depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;
// solhint-disable-next-line func-named-parameters
emit DepositInitiated(l2TxHash, msg.sender, _l2Receiver, _l1Token, _amount);
}

Expand Down Expand Up @@ -222,13 +223,13 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
require(!isWithdrawalFinalized[_l2BatchNumber][_l2MessageIndex], "pw");
// We don't need to set finalizeWithdrawal here, as we set it in the shared bridge

(address l1Receiver, address l1Token, uint256 amount) = sharedBridge.finalizeWithdrawalLegacyErc20Bridge(
_l2BatchNumber,
_l2MessageIndex,
_l2TxNumberInBatch,
_message,
_merkleProof
);
(address l1Receiver, address l1Token, uint256 amount) = sharedBridge.finalizeWithdrawalLegacyErc20Bridge({
_l2BatchNumber: _l2BatchNumber,
_l2MessageIndex: _l2MessageIndex,
_l2TxNumberInBatch: _l2TxNumberInBatch,
_message: _message,
_merkleProof: _merkleProof
});
emit WithdrawalFinalized(l1Receiver, l1Token, amount);
}
}
14 changes: 7 additions & 7 deletions l1-contracts/contracts/bridge/L1SharedBridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -501,13 +501,13 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Initializable, Owna
});
}

bool success = bridgehub.proveL2MessageInclusion(
_chainId,
_messageParams.l2BatchNumber,
_messageParams.l2MessageIndex,
l2ToL1Message,
_merkleProof
);
bool success = bridgehub.proveL2MessageInclusion({
_chainId: _chainId,
_batchNumber: _messageParams.l2BatchNumber,
_index: _messageParams.l2MessageIndex,
_message: l2ToL1Message,
_proof: _merkleProof
});
require(success, "ShB withd w proof"); // withdrawal wrong proof
}

Expand Down
14 changes: 7 additions & 7 deletions l1-contracts/contracts/bridgehub/Bridgehub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,13 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2Step {
stateTransitionManager[_chainId] = _stateTransitionManager;
baseToken[_chainId] = _baseToken;

IStateTransitionManager(_stateTransitionManager).createNewChain(
_chainId,
_baseToken,
address(sharedBridge),
_admin,
_initData
);
IStateTransitionManager(_stateTransitionManager).createNewChain({
_chainId: _chainId,
_baseToken: _baseToken,
_sharedBridge: address(sharedBridge),
_admin: _admin,
_diamondCut: _initData
});

emit NewChain(_chainId, _stateTransitionManager, _admin);
return _chainId;
Expand Down
17 changes: 15 additions & 2 deletions l1-contracts/contracts/dev-contracts/test/ReenterL1ERC20Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,14 @@ contract ReenterL1ERC20Bridge {

fallback() external payable {
if (functionToCall == FunctionToCall.LegacyDeposit) {
l1Erc20Bridge.deposit(address(0), address(0), 0, 0, 0);
l1Erc20Bridge.deposit({
_l2Receiver: address(0),
_l1Token: address(0),
_amount: 0,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0,
_refundRecipient: address(0)
});
} else if (functionToCall == FunctionToCall.Deposit) {
l1Erc20Bridge.deposit({
_l2Receiver: address(0),
Expand All @@ -53,7 +60,13 @@ contract ReenterL1ERC20Bridge {
});
} else if (functionToCall == FunctionToCall.FinalizeWithdrawal) {
bytes32[] memory merkleProof;
l1Erc20Bridge.finalizeWithdrawal(0, 0, 0, bytes(""), merkleProof);
l1Erc20Bridge.finalizeWithdrawal({
_l2BatchNumber: 0,
_l2MessageIndex: 0,
_l2TxNumberInBatch: 0,
_message: bytes(""),
_merkleProof: merkleProof
});
} else {
revert("Unset function to call");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ contract MailboxFacet is ZkSyncStateTransitionBase, IMailbox {
);

// Data that is needed for the operator to simulate priority queue offchain
// solhint-disable-next-line func-named-parameters
emit NewPriorityRequest(
_priorityOpParams.txId,
canonicalTxHash,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,15 @@ contract ExperimentalBridgeTest is Test {
abi.encode(true)
);

assertTrue(bridgeHub.proveL2MessageInclusion(mockChainId, mockBatchNumber, mockIndex, l2Message, mockProof));
assertTrue(
bridgeHub.proveL2MessageInclusion({
_chainId: mockChainId,
_batchNumber: mockBatchNumber,
_index: mockIndex,
_message: l2Message,
_proof: mockProof
})
);
vm.clearMockedCalls();
}

Expand Down Expand Up @@ -490,7 +498,15 @@ contract ExperimentalBridgeTest is Test {
abi.encode(true)
);

assertTrue(bridgeHub.proveL2LogInclusion(mockChainId, mockBatchNumber, mockIndex, l2Log, mockProof));
assertTrue(
bridgeHub.proveL2LogInclusion({
_chainId: mockChainId,
_batchNumber: mockBatchNumber,
_index: mockIndex,
_log: l2Log,
_proof: mockProof
})
);
vm.clearMockedCalls();
}

Expand Down Expand Up @@ -969,7 +985,15 @@ contract ExperimentalBridgeTest is Test {
abi.encode(true)
);

assertTrue(bridgeHub.proveL2MessageInclusion(mockChainId, mockBatchNumber, mockIndex, l2Message, mockProof));
assertTrue(
bridgeHub.proveL2MessageInclusion({
_chainId: mockChainId,
_batchNumber: mockBatchNumber,
_index: mockIndex,
_message: l2Message,
_proof: mockProof
})
);
}

function test_proveL2LogInclusion_old(
Expand Down Expand Up @@ -1011,7 +1035,15 @@ contract ExperimentalBridgeTest is Test {
abi.encode(true)
);

assertTrue(bridgeHub.proveL2LogInclusion(mockChainId, mockBatchNumber, mockIndex, l2Log, mockProof));
assertTrue(
bridgeHub.proveL2LogInclusion({
_chainId: mockChainId,
_batchNumber: mockBatchNumber,
_index: mockIndex,
_log: l2Log,
_proof: mockProof
})
);
}

function test_proveL1ToL2TransactionStatus_old(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ contract ClaimFailedDepositTest is L1Erc20BridgeTest {
assertEq(depositedAmountAfterDeposit, amount);

vm.prank(alice);
// solhint-disable-next-line func-named-parameters
vm.expectEmit(true, true, true, true, address(bridge));
emit ClaimedFailedDeposit(alice, address(token), amount);
bytes32[] memory merkleProof;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,57 @@ contract DepositTest is L1Erc20BridgeTest {

function test_RevertWhen_legacyDepositAmountIsZero() public {
vm.expectRevert(bytes("0T"));
bridge.deposit(randomSigner, address(token), 0, 0, 0);
bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: address(token),
_amount: 0,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function test_RevertWhen_depositTokenIsNotContract() public {
vm.expectRevert();
bridge.deposit(randomSigner, makeAddr("EOA"), 1, 0, 0);
bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: makeAddr("EOA"),
_amount: 1,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function test_RevertWhen_legacyDepositTokenIsNotContract() public {
vm.expectRevert();
bridge.deposit(randomSigner, makeAddr("EOA"), 1, 0, 0);
bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: makeAddr("EOA"),
_amount: 1,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function test_RevertWhen_depositTokenTransferFailed() public {
vm.expectRevert("ERC20: insufficient allowance");
bridge.deposit(randomSigner, address(token), 1, 0, 0);
bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: address(token),
_amount: 1,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function test_RevertWhen_legacyDepositTokenTransferFailed() public {
vm.expectRevert("ERC20: insufficient allowance");
bridge.deposit(randomSigner, address(token), 1, 0, 0);
bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: address(token),
_amount: 1,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function test_RevertWhen_depositTransferAmountIsDifferent() public {
Expand All @@ -56,7 +86,13 @@ contract DepositTest is L1Erc20BridgeTest {
feeOnTransferToken.approve(address(bridge), amount);
vm.expectRevert(bytes("3T"));
vm.prank(alice);
bridge.deposit(randomSigner, address(feeOnTransferToken), amount, 0, 0);
bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: address(feeOnTransferToken),
_amount: amount,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function test_RevertWhen_legacyDepositTransferAmountIsDifferent() public {
Expand All @@ -65,15 +101,23 @@ contract DepositTest is L1Erc20BridgeTest {
feeOnTransferToken.approve(address(bridge), amount);
vm.expectRevert(bytes("3T"));
vm.prank(alice);
bridge.deposit(randomSigner, address(feeOnTransferToken), amount, 0, 0);
bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: address(feeOnTransferToken),
_amount: amount,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function test_depositSuccessfully() public {
uint256 amount = 8;
vm.prank(alice);
token.approve(address(bridge), amount);
vm.prank(alice);
// solhint-disable-next-line func-named-parameters
vm.expectEmit(true, true, true, true, address(bridge));
// solhint-disable-next-line func-named-parameters
emit DepositInitiated(dummyL2DepositTxHash, alice, randomSigner, address(token), amount);
bytes32 txHash = bridge.deposit({
_l2Receiver: randomSigner,
Expand All @@ -97,9 +141,17 @@ contract DepositTest is L1Erc20BridgeTest {
vm.prank(alice);
token.approve(address(bridge), amount);
vm.prank(alice);
// solhint-disable-next-line func-named-parameters
vm.expectEmit(true, true, true, true, address(bridge));
// solhint-disable-next-line func-named-parameters
emit DepositInitiated(dummyL2DepositTxHash, alice, randomSigner, address(token), amount);
bytes32 txHash = bridge.deposit(randomSigner, address(token), amount, 0, 0);
bytes32 txHash = bridge.deposit({
_l2Receiver: randomSigner,
_l1Token: address(token),
_amount: amount,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
assertEq(txHash, dummyL2DepositTxHash);

uint256 depositedAmount = bridge.depositAmount(alice, address(token), dummyL2DepositTxHash);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ contract FinalizeWithdrawalTest is L1Erc20BridgeTest {

vm.expectRevert(bytes("pw"));
bytes32[] memory merkleProof;
bridge.finalizeWithdrawal(l2BatchNumber, l2MessageIndex, 0, "", merkleProof);
bridge.finalizeWithdrawal({
_l2BatchNumber: l2BatchNumber,
_l2MessageIndex: l2MessageIndex,
_l2TxNumberInBatch: 0,
_message: "",
_merkleProof: merkleProof
});
}

function test_finalizeWithdrawalSuccessfully() public {
Expand All @@ -37,10 +43,17 @@ contract FinalizeWithdrawalTest is L1Erc20BridgeTest {
dummySharedBridge.setDataToBeReturnedInFinalizeWithdrawal(alice, address(token), amount);

vm.prank(alice);
// solhint-disable-next-line func-named-parameters
vm.expectEmit(true, true, true, true, address(bridge));
emit WithdrawalFinalized(alice, address(token), amount);
bytes32[] memory merkleProof;
bridge.finalizeWithdrawal(l2BatchNumber, l2MessageIndex, 0, "", merkleProof);
bridge.finalizeWithdrawal({
_l2BatchNumber: l2BatchNumber,
_l2MessageIndex: l2MessageIndex,
_l2TxNumberInBatch: 0,
_message: "",
_merkleProof: merkleProof
});

// withdrawal finalization should be handled in the shared bridge, so it shouldn't
// change in the L1 ERC20 bridge after finalization.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,13 @@ contract ReentrancyTest is L1Erc20BridgeTest {

vm.prank(alice);
vm.expectRevert(bytes("r1"));
bridgeReenterItself.deposit(randomSigner, address(token), amount, 0, 0);
bridgeReenterItself.deposit({
_l2Receiver: randomSigner,
_l1Token: address(token),
_amount: amount,
_l2TxGasLimit: 0,
_l2TxGasPerPubdataByte: 0
});
}

function _claimFailedDepositExpectRevertOnReentrancy() internal {
Expand Down Expand Up @@ -67,7 +73,13 @@ contract ReentrancyTest is L1Erc20BridgeTest {
vm.prank(alice);
vm.expectRevert(bytes("r1"));
bytes32[] memory merkleProof;
bridgeReenterItself.finalizeWithdrawal(l2BatchNumber, l2MessageIndex, 0, "", merkleProof);
bridgeReenterItself.finalizeWithdrawal({
_l2BatchNumber: l2BatchNumber,
_l2MessageIndex: l2MessageIndex,
_l2TxNumberInBatch: 0,
_message: "",
_merkleProof: merkleProof
});
}

function test_depositReenterDeposit() public {
Expand Down
Loading

0 comments on commit a52ddf6

Please sign in to comment.