Skip to content

Commit

Permalink
Audittens audit C01 (#11)
Browse files Browse the repository at this point in the history
I did a bit more than suggested: used the same function everywhere for
checking whether priority queue is active + put into the same file the
suggested operation to ensure that it is easier to track the invariant
  • Loading branch information
StanislavBreadless authored Nov 28, 2024
1 parent 7198b54 commit 4039eab
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,7 @@ contract AdminFacet is ZKChainBase, IAdmin {
} else {
s.priorityTree.initFromCommitment(_commitment.priorityTree);
}
_forceDeactivateQueue();

s.l2SystemContractsUpgradeTxHash = _commitment.l2SystemContractsUpgradeTxHash;
s.l2SystemContractsUpgradeBatchNumber = _commitment.l2SystemContractsUpgradeBatchNumber;
Expand Down Expand Up @@ -366,7 +367,7 @@ contract AdminFacet is ZKChainBase, IAdmin {
/// @dev Note, that this is a getter method helpful for debugging and should not be relied upon by clients.
/// @return commitment The commitment for the chain.
function prepareChainCommitment() public view returns (ZKChainCommitment memory commitment) {
require(s.priorityQueue.getFirstUnprocessedPriorityTx() >= s.priorityTree.startIndex, "PQ not ready");
require(!_isPriorityQueueActive(), "PQ not ready");

commitment.totalBatchesCommitted = s.totalBatchesCommitted;
commitment.totalBatchesVerified = s.totalBatchesVerified;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -441,13 +441,13 @@ contract ExecutorFacet is ZKChainBase, IExecutor {
require(batchesData.length == priorityOpsData.length, "bp");

for (uint256 i = 0; i < nBatches; i = i.uncheckedInc()) {
if (s.priorityTree.startIndex <= s.priorityQueue.getFirstUnprocessedPriorityTx()) {
_executeOneBatch(batchesData[i], priorityOpsData[i], i);
} else {
if (_isPriorityQueueActive()) {
require(priorityOpsData[i].leftPath.length == 0, "le");
require(priorityOpsData[i].rightPath.length == 0, "re");
require(priorityOpsData[i].itemHashes.length == 0, "ih");
_executeOneBatch(batchesData[i], i);
} else {
_executeOneBatch(batchesData[i], priorityOpsData[i], i);
}
emit BlockExecution(batchesData[i].batchNumber, batchesData[i].batchHash, batchesData[i].commitment);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ contract GettersFacet is ZKChainBase, IGetters, ILegacyGetters {

/// @inheritdoc IGetters
function getFirstUnprocessedPriorityTx() external view returns (uint256) {
if (s.priorityQueue.getFirstUnprocessedPriorityTx() >= s.priorityTree.startIndex) {
return s.priorityTree.getFirstUnprocessedPriorityTx();
} else {
if (_isPriorityQueueActive()) {
return s.priorityQueue.getFirstUnprocessedPriorityTx();
} else {
return s.priorityTree.getFirstUnprocessedPriorityTx();
}
}

Expand All @@ -126,13 +126,18 @@ contract GettersFacet is ZKChainBase, IGetters, ILegacyGetters {

/// @inheritdoc IGetters
function getPriorityQueueSize() external view returns (uint256) {
if (s.priorityQueue.getFirstUnprocessedPriorityTx() >= s.priorityTree.startIndex) {
return s.priorityTree.getSize();
} else {
if (_isPriorityQueueActive()) {
return s.priorityQueue.getSize();
} else {
return s.priorityTree.getSize();
}
}

/// @inheritdoc IGetters
function isPriorityQueueActive() external view returns (bool) {
return _isPriorityQueueActive();
}

/// @inheritdoc IGetters
function isValidator(address _address) external view returns (bool) {
return s.validators[_address];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -494,10 +494,10 @@ contract MailboxFacet is ZKChainBase, IMailbox {
}

function _nextPriorityTxId() internal view returns (uint256) {
if (s.priorityQueue.getFirstUnprocessedPriorityTx() >= s.priorityTree.startIndex) {
return s.priorityTree.getTotalPriorityTxs();
} else {
if (_isPriorityQueueActive()) {
return s.priorityQueue.getTotalPriorityTxs();
} else {
return s.priorityTree.getTotalPriorityTxs();
}
}

Expand Down Expand Up @@ -570,7 +570,7 @@ contract MailboxFacet is ZKChainBase, IMailbox {
}

function _writePriorityOpHash(bytes32 _canonicalTxHash, uint64 _expirationTimestamp) internal {
if (s.priorityTree.startIndex > s.priorityQueue.getFirstUnprocessedPriorityTx()) {
if (_isPriorityQueueActive()) {
s.priorityQueue.pushBack(
PriorityOperation({
canonicalTxHash: _canonicalTxHash,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,29 @@ contract ZKChainBase is ReentrancyGuard {
_;
}

/// @notice Returns whether the priority queue is still active, i.e.
/// the chain has not processed all transactions from it
function _isPriorityQueueActive() internal view returns (bool) {
return s.priorityQueue.getFirstUnprocessedPriorityTx() < s.priorityTree.startIndex;
}

/// @notice Ensures that the queue is deactivated. Should be invoked
/// whenever the chain migrates to another settlement layer.
function _forceDeactivateQueue() internal {
// We double check whether it is still active mainly to prevent
// overriding `tail`/`head` on L1 deployment.
if (_isPriorityQueueActive()) {
uint256 startIndex = s.priorityTree.startIndex;
s.priorityQueue.head = startIndex;
s.priorityQueue.tail = startIndex;
}
}

function _getTotalPriorityTxs() internal view returns (uint256) {
if (s.priorityQueue.getFirstUnprocessedPriorityTx() >= s.priorityTree.startIndex) {
return s.priorityTree.getTotalPriorityTxs();
} else {
if (_isPriorityQueueActive()) {
return s.priorityQueue.getTotalPriorityTxs();
} else {
return s.priorityTree.getTotalPriorityTxs();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ interface IGetters is IZKChainBase {
/// @return The root hash of the priority tree
function getPriorityTreeRoot() external view returns (bytes32);

/// @return Whether the priority queue is active, i.e. whether new priority operations are appended to it.
/// Once the chain processes all the transaction that were present in the priority queue, all the L1->L2 related
/// operations will start to get done using the priority tree.
function isPriorityQueueActive() external view returns (bool);

/// @notice The function that returns the first unprocessed priority transaction.
/// @dev Returns zero if and only if no operations were processed from the queue.
/// @dev If all the transactions were processed, it will return the last processed index, so
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,31 @@ import {IZKChain} from "contracts/state-transition/chain-interfaces/IZKChain.sol
import {SystemContractsArgs} from "./_SharedL2ContractL1DeployerUtils.sol";

import {DeployUtils} from "deploy-scripts/DeployUtils.s.sol";
import {GettersFacet} from "contracts/state-transition/chain-deps/facets/Getters.sol";
import {ZKChainCommitment} from "contracts/common/Config.sol";

abstract contract L2GatewayTestAbstract is Test, SharedL2ContractDeployer {
function test_gatewayShouldFinalizeDeposit() public {
finalizeDeposit();
require(l2Bridgehub.ctmAssetIdFromAddress(address(chainTypeManager)) == ctmAssetId, "ctmAssetId mismatch");
require(l2Bridgehub.ctmAssetIdFromChainId(mintChainId) == ctmAssetId, "ctmAssetIdFromChainId mismatch");

address diamondProxy = l2Bridgehub.getZKChain(mintChainId);
require(!GettersFacet(diamondProxy).isPriorityQueueActive(), "Priority queue must not be active");
}

function test_gatewayNonEmptyPriorityQueueMigration() public {
ZKChainCommitment memory commitment = abi.decode(exampleChainCommitment, (ZKChainCommitment));

// Some non-zero value which would be the case if a chain existed before the
// priority tree was added
commitment.priorityTree.startIndex = 101;
commitment.priorityTree.nextLeafIndex = 102;

finalizeDepositWithCustomCommitment(abi.encode(commitment));

address diamondProxy = l2Bridgehub.getZKChain(mintChainId);
require(!GettersFacet(diamondProxy).isPriorityQueueActive(), "Priority queue must not be active");
}

function test_forwardToL3OnGateway() public {
Expand Down Expand Up @@ -66,7 +85,11 @@ abstract contract L2GatewayTestAbstract is Test, SharedL2ContractDeployer {
}

function finalizeDeposit() public {
bytes memory chainData = exampleChainCommitment;
finalizeDepositWithCustomCommitment(exampleChainCommitment);
}

function finalizeDepositWithCustomCommitment(bytes memory chainCommitment) public {
bytes memory chainData = chainCommitment;
bytes memory ctmData = abi.encode(
baseTokenAssetId,
ownerWallet,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol";
import {L2WrappedBaseToken} from "contracts/bridge/L2WrappedBaseToken.sol";
import {L2SharedBridgeLegacy} from "contracts/bridge/L2SharedBridgeLegacy.sol";
import {DataEncoding} from "contracts/common/libraries/DataEncoding.sol";
import {MailboxFacet} from "contracts/state-transition/chain-deps/facets/Mailbox.sol";
import {AdminFacet} from "contracts/state-transition/chain-deps/facets/Admin.sol";
import {BridgehubL2TransactionRequest} from "contracts/common/Messaging.sol";

import {IChainTypeManager} from "contracts/state-transition/IChainTypeManager.sol";
import {IZKChain} from "contracts/state-transition/chain-interfaces/IZKChain.sol";
Expand Down Expand Up @@ -126,6 +129,8 @@ abstract contract SharedL2ContractDeployer is Test, DeployUtils {
}

function getExampleChainCommitment() internal returns (bytes memory) {
address chainAdmin = makeAddr("chainAdmin");

vm.mockCall(
L2_ASSET_ROUTER_ADDR,
abi.encodeWithSelector(IL1AssetRouter.L1_NULLIFIER.selector),
Expand All @@ -140,10 +145,33 @@ abstract contract SharedL2ContractDeployer is Test, DeployUtils {
address chainAddress = chainTypeManager.createNewChain(
ERA_CHAIN_ID + 1,
baseTokenAssetId,
address(0x1),
chainAdmin,
abi.encode(config.contracts.diamondCutData, generatedData.forceDeploymentsData),
new bytes[](0)
);

vm.prank(chainAdmin);
AdminFacet(chainAddress).setTokenMultiplier(1, 1);

// Now, let's also append a priority transaction for a more representative example
bytes[] memory deps = new bytes[](0);

vm.prank(address(l2Bridgehub));
MailboxFacet(chainAddress).bridgehubRequestL2Transaction(
BridgehubL2TransactionRequest({
sender: address(0),
contractL2: address(0),
// Just a giant number so it is always enough
mintValue: 1 ether,
l2Value: 10,
l2Calldata: hex"",
l2GasLimit: 72_000_000,
l2GasPerPubdataByteLimit: 800,
factoryDeps: deps,
refundRecipient: address(0)
})
);

exampleChainCommitment = abi.encode(IZKChain(chainAddress).prepareChainCommitment());
}

Expand Down

0 comments on commit 4039eab

Please sign in to comment.