Skip to content

Commit

Permalink
Fix audittens L07 (#33)
Browse files Browse the repository at this point in the history
  • Loading branch information
StanislavBreadless authored Dec 10, 2024
1 parent 1982d82 commit 02d6fbb
Show file tree
Hide file tree
Showing 7 changed files with 44 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
function withdrawToken(address _l2NativeToken, bytes memory _assetData) public returns (bytes32) {
bytes32 recordedAssetId = INativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).assetId(_l2NativeToken);
uint256 recordedOriginChainId = INativeTokenVault(L2_NATIVE_TOKEN_VAULT_ADDR).originChainId(recordedAssetId);
if (recordedOriginChainId != block.chaind && recordedOriginChainId != 0) {
if (recordedOriginChainId != block.chainid && recordedOriginChainId != 0) {
revert AssetIdNotSupported(recordedAssetId);
}
bytes32 assetId = _ensureTokenRegisteredWithNTV(_l2NativeToken);
Expand Down
2 changes: 2 additions & 0 deletions l1-contracts/contracts/common/L1ContractErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,8 @@ error AssetRouterAllowanceNotZero();
error BurningNativeWETHNotSupported();
// 0xb20b58ce
error NoLegacySharedBridge();
// 0x8e3ce3cb
error TooHighDeploymentNonce();

enum SharedBridgeKey {
PostUpgradeFirstBatch,
Expand Down
17 changes: 17 additions & 0 deletions l1-contracts/contracts/common/libraries/L2ContractHelper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ library L2ContractHelper {
/// @dev The prefix used to create CREATE2 addresses.
bytes32 private constant CREATE2_PREFIX = keccak256("zksyncCreate2");

/// @dev Prefix used during derivation of account addresses using CREATE
bytes32 private constant CREATE_PREFIX = 0x63bae3a9951d38e8a3fbb7b70909afc1200610fc5bc55ade242f815974674f23;

/// @notice Sends L2 -> L1 arbitrary-long message through the system contract messenger.
/// @param _message Data to be sent to L1.
/// @return keccak256 hash of the sent message.
Expand Down Expand Up @@ -131,6 +134,20 @@ library L2ContractHelper {
return address(uint160(uint256(data)));
}

/// @notice Calculates the address of a deployed contract via create
/// @param _sender The account that deploys the contract.
/// @param _senderNonce The deploy nonce of the sender's account.
/// NOTE: L2 create derivation is different from L1 derivation!
function computeCreateAddress(address _sender, uint256 _senderNonce) internal pure returns (address) {
// No collision is possible with the Ethereum's CREATE, since
// the prefix begins with 0x63....
bytes32 hash = keccak256(
bytes.concat(CREATE_PREFIX, bytes32(uint256(uint160(_sender))), bytes32(_senderNonce))
);

return address(uint160(uint256(hash)));
}

/// @notice Hashes the L2 bytecodes and returns them in the format in which they are processed by the bootloader
function hashFactoryDeps(bytes[] memory _factoryDeps) internal pure returns (uint256[] memory hashedFactoryDeps) {
uint256 factoryDepsLen = _factoryDeps.length;
Expand Down
8 changes: 6 additions & 2 deletions l1-contracts/contracts/governance/L2AdminFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ contract L2AdminFactory {
/// @notice Deploys a new L2 admin contract.
/// @return admin The address of the deployed admin contract.
// solhint-disable-next-line gas-calldata-parameters
function deployAdmin(address[] memory _additionalRestrictions, bytes32 _salt) external returns (address admin) {
function deployAdmin(address[] memory _additionalRestrictions) external returns (address admin) {
// Even though the chain admin will likely perform similar checks,
// we keep those here just in case, since it is not expensive, while allowing to fail fast.
_validateRestrctions(_additionalRestrictions);
Expand All @@ -45,7 +45,11 @@ contract L2AdminFactory {
restrictions[requiredRestrictions.length + i] = _additionalRestrictions[i];
}

admin = address(new ChainAdmin{salt: _salt}(restrictions));
// Note, that we are using CREATE instead of CREATE2 to prevent
// an attack where malicious deployer could select malicious `seed1` and `seed2` where
// this factory with `seed1` produces the same address as some other random factory with `seed2`,
// allowing to deploy a malicious contract.
admin = address(new ChainAdmin(restrictions));
}

/// @notice Checks that the provided list of restrictions is correct.
Expand Down
24 changes: 13 additions & 11 deletions l1-contracts/contracts/governance/PermanentRestriction.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

pragma solidity 0.8.24;

import {CallNotAllowed, RemovingPermanentRestriction, ZeroAddress, UnallowedImplementation, AlreadyWhitelisted, NotAllowed} from "../common/L1ContractErrors.sol";
import {TooHighDeploymentNonce, CallNotAllowed, RemovingPermanentRestriction, ZeroAddress, UnallowedImplementation, AlreadyWhitelisted, NotAllowed} from "../common/L1ContractErrors.sol";

import {L2TransactionRequestTwoBridgesOuter, BridgehubBurnCTMAssetData} from "../bridgehub/IBridgehub.sol";
import {Ownable2StepUpgradeable} from "@openzeppelin/contracts-upgradeable-v4/access/Ownable2StepUpgradeable.sol";
Expand All @@ -24,6 +24,11 @@ import {IPermanentRestriction} from "./IPermanentRestriction.sol";
/// has at least this amount.
uint256 constant MIN_GAS_FOR_FALLABLE_CALL = 5_000_000;

/// @dev The value up to which the nonces of the L2AdminDeployer could be used. This is needed
/// to limit the impact of the birthday paradox attack, where an attack could craft a malicious
/// address on L1.
uint256 constant MAX_ALLOWED_NONCE = (1 << 48);

/// @title PermanentRestriction contract
/// @author Matter Labs
/// @custom:security-contact [email protected]
Expand Down Expand Up @@ -98,18 +103,15 @@ contract PermanentRestriction is Restriction, IPermanentRestriction, Ownable2Ste
}

/// @notice Whitelists a certain L2 admin.
/// @param deploymentSalt The salt for the deployment.
/// @param l2BytecodeHash The hash of the L2 bytecode.
/// @param constructorInputHash The hash of the constructor data for the deployment.
function allowL2Admin(bytes32 deploymentSalt, bytes32 l2BytecodeHash, bytes32 constructorInputHash) external {
/// @param deploymentNonce The deployment nonce of the `L2_ADMIN_FACTORY` used for the deployment.
function allowL2Admin(uint256 deploymentNonce) external {
if (deploymentNonce > MAX_ALLOWED_NONCE) {
revert TooHighDeploymentNonce();
}

// We do not do any additional validations for constructor data or the bytecode,
// we expect that only admins of the allowed format are to be deployed.
address expectedAddress = L2ContractHelper.computeCreate2Address(
L2_ADMIN_FACTORY,
deploymentSalt,
l2BytecodeHash,
constructorInputHash
);
address expectedAddress = L2ContractHelper.computeCreateAddress(L2_ADMIN_FACTORY, deploymentNonce);

if (allowedL2Admins[expectedAddress]) {
revert AlreadyWhitelisted(expectedAddress);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -345,16 +345,11 @@ contract PermanentRestrictionTest is ChainTypeManagerTest {
}

function test_validateMigrationToL2() public {
address expectedAddress = L2ContractHelper.computeCreate2Address(
L2_FACTORY_ADDR,
bytes32(0),
bytes32(0),
bytes32(0)
);
address expectedAddress = L2ContractHelper.computeCreateAddress(L2_FACTORY_ADDR, uint256(0));

vm.expectEmit(true, false, false, true);
emit IPermanentRestriction.AllowL2Admin(expectedAddress);
permRestriction.allowL2Admin(bytes32(0), bytes32(0), bytes32(0));
permRestriction.allowL2Admin(uint256(0));

Call memory call = _encodeMigraationCall(true, true, true, true, true, expectedAddress);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract L2AdminFactoryTest is Test {
additionalRestrictions[0] = invalidRestriction;

vm.expectRevert(abi.encodeWithSelector(NotARestriction.selector, address(invalidRestriction)));
factory.deployAdmin(additionalRestrictions, bytes32(0));
factory.deployAdmin(additionalRestrictions);
}

function testL2AdminFactory() public {
Expand All @@ -58,9 +58,7 @@ contract L2AdminFactoryTest is Test {
allRestrictions[0] = requiredRestrictions[0];
allRestrictions[1] = additionalRestrictions[0];

bytes32 salt = keccak256("salt");

address admin = factory.deployAdmin(additionalRestrictions, salt);
address admin = factory.deployAdmin(additionalRestrictions);

// Now, we need to check whether it would be able to accept such an admin
PermanentRestriction restriction = new PermanentRestriction(IBridgehub(address(0)), address(factory));
Expand All @@ -72,6 +70,6 @@ contract L2AdminFactoryTest is Test {

vm.expectEmit(true, false, false, true);
emit IPermanentRestriction.AllowL2Admin(admin);
restriction.allowL2Admin(salt, codeHash, keccak256(abi.encode(allRestrictions)));
restriction.allowL2Admin(uint256(0));
}
}

0 comments on commit 02d6fbb

Please sign in to comment.