Skip to content

Commit

Permalink
Merge pull request #941 from matter-labs/gw-audit-2-sls
Browse files Browse the repository at this point in the history 
Gw audit 2 sls
  • Loading branch information
@kelemeno
kelemeno authored Oct 15, 2024
2 parents ebe620f + 5ea47e1 commit 7198b54
Show file tree
Hide file tree
Showing 14 changed files with 156 additions and 87 deletions.
13 changes: 5 additions & 8 deletions l1-contracts/contracts/bridge/BridgedStandardERC20.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,7 @@ contract BridgedStandardERC20 is ERC20PermitUpgradeable, IBridgedStandardToken,
/// @param _originToken Address of the origin token that can be deposited to mint this bridged token
/// @param _data The additional data that the L1 bridge provide for initialization.
/// In this case, it is packed `name`/`symbol`/`decimals` of the L1 token.
function bridgeInitialize(
bytes32 _assetId,
address _originToken,
bytes calldata _data
) external initializer returns (uint256) {
function bridgeInitialize(bytes32 _assetId, address _originToken, bytes calldata _data) external initializer {
if (_originToken == address(0)) {
revert ZeroAddress();
}
Expand All @@ -100,8 +96,10 @@ contract BridgedStandardERC20 is ERC20PermitUpgradeable, IBridgedStandardToken,
nativeTokenVault = msg.sender;

// We parse the data exactly as they were created on the L1 bridge
(uint256 chainId, bytes memory nameBytes, bytes memory symbolBytes, bytes memory decimalsBytes) = DataEncoding
.decodeTokenData(_data);
// slither-disable-next-line unused-return
(, bytes memory nameBytes, bytes memory symbolBytes, bytes memory decimalsBytes) = DataEncoding.decodeTokenData(
_data
);

ERC20Getters memory getters;
string memory decodedName;
Expand Down Expand Up @@ -143,7 +141,6 @@ contract BridgedStandardERC20 is ERC20PermitUpgradeable, IBridgedStandardToken,

availableGetters = getters;
emit BridgeInitialize(_originToken, decodedName, decodedSymbol, decimals_);
return chainId;
}

/// @notice A method to be called by the governor to update the token's metadata.
Expand Down
21 changes: 15 additions & 6 deletions l1-contracts/contracts/bridge/L1ERC20Bridge.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {IL1AssetRouter} from "./asset-router/IL1AssetRouter.sol";
import {L2ContractHelper} from "../common/libraries/L2ContractHelper.sol";
import {ReentrancyGuard} from "../common/ReentrancyGuard.sol";

import {EmptyDeposit, WithdrawalAlreadyFinalized, TokensWithFeesNotSupported, ETHDepositNotSupported} from "../common/L1ContractErrors.sol";
import {EmptyDeposit, WithdrawalAlreadyFinalized, TokensWithFeesNotSupported, ETHDepositNotSupported, ApprovalFailed} from "../common/L1ContractErrors.sol";
import {ETH_TOKEN_ADDRESS} from "../common/Config.sol";

/// @author Matter Labs
Expand Down Expand Up @@ -188,7 +188,7 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
if (_l1Token == ETH_TOKEN_ADDRESS) {
revert ETHDepositNotSupported();
}
uint256 amount = _depositFundsToAssetRouter(msg.sender, IERC20(_l1Token), _amount);
uint256 amount = _approveFundsToAssetRouter(msg.sender, IERC20(_l1Token), _amount);
if (amount != _amount) {
// The token has non-standard transfer logic
revert TokensWithFeesNotSupported();
Expand All @@ -203,6 +203,11 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {
_l2TxGasPerPubdataByte: _l2TxGasPerPubdataByte,
_refundRecipient: _refundRecipient
});
// clearing approval
bool success = IERC20(_l1Token).approve(address(L1_ASSET_ROUTER), 0);
if (!success) {
revert ApprovalFailed();
}
depositAmount[msg.sender][_l1Token][l2TxHash] = _amount;
emit DepositInitiated({
l2DepositTxHash: l2TxHash,
Expand All @@ -219,10 +224,14 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard {

/// @dev Transfers tokens from the depositor address to the native token vault address.
/// @return The difference between the contract balance before and after the transferring of funds.
function _depositFundsToAssetRouter(address _from, IERC20 _token, uint256 _amount) internal returns (uint256) {
uint256 balanceBefore = _token.balanceOf(address(L1_ASSET_ROUTER));
_token.safeTransferFrom(_from, address(L1_ASSET_ROUTER), _amount);
uint256 balanceAfter = _token.balanceOf(address(L1_ASSET_ROUTER));
function _approveFundsToAssetRouter(address _from, IERC20 _token, uint256 _amount) internal returns (uint256) {
uint256 balanceBefore = _token.balanceOf(address(this));
_token.safeTransferFrom(_from, address(this), _amount);
bool success = _token.approve(address(L1_ASSET_ROUTER), _amount);
if (!success) {
revert ApprovalFailed();
}
uint256 balanceAfter = _token.balanceOf(address(this));

return balanceAfter - balanceBefore;
}
Expand Down
11 changes: 9 additions & 2 deletions l1-contracts/contracts/bridge/asset-router/L1AssetRouter.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,10 +404,17 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {

// Do the transfer if allowance to Shared bridge is bigger than amount
// And if there is not enough allowance for the NTV
if (
bool weCanTransfer = false;
if (l1Token.allowance(address(legacyBridge), address(this)) >= _amount) {
_originalCaller = address(legacyBridge);
weCanTransfer = true;
} else if (
l1Token.allowance(_originalCaller, address(this)) >= _amount &&
l1Token.allowance(_originalCaller, address(nativeTokenVault)) < _amount
) {
weCanTransfer = true;
}
if (weCanTransfer) {
// slither-disable-next-line arbitrary-send-erc20
l1Token.safeTransferFrom(_originalCaller, address(nativeTokenVault), _amount);
return true;
Expand Down Expand Up @@ -542,7 +549,7 @@ contract L1AssetRouter is AssetRouterBase, IL1AssetRouter, ReentrancyGuard {
// Save the deposited amount to claim funds on L1 if the deposit failed on L2
L1_NULLIFIER.bridgehubConfirmL2TransactionForwarded(
ERA_CHAIN_ID,
keccak256(abi.encode(_originalCaller, _l1Token, _amount)),
DataEncoding.encodeLegacyTxDataHash(_originalCaller, _l1Token, _amount),
txHash
);

Expand Down
1 change: 1 addition & 0 deletions l1-contracts/contracts/bridge/asset-router/L2AssetRouter.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ contract L2AssetRouter is AssetRouterBase, IL2AssetRouter {
}

/// @dev Disable the initialization to prevent Parity hack.
/// @dev this contract is deployed in the L2GenesisUpgrade, and is meant as direct deployment without a proxy.
/// @param _l1AssetRouter The address of the L1 Bridge contract.
constructor(
uint256 _l1ChainId,
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/contracts/bridge/ntv/L1NativeTokenVault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ contract L1NativeTokenVault is IL1NativeTokenVault, IL1AssetHandler, NativeToken
}
}

function _deployBeaconProxy(bytes32 _salt) internal override returns (BeaconProxy proxy) {
function _deployBeaconProxy(bytes32 _salt, uint256) internal override returns (BeaconProxy proxy) {
// Use CREATE2 to deploy the BeaconProxy
address proxyAddress = Create2.deploy(
0,
Expand Down
70 changes: 48 additions & 22 deletions l1-contracts/contracts/bridge/ntv/L2NativeTokenVault.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault {
bytes32 internal l2TokenProxyBytecodeHash;

/// @notice Initializes the bridge contract for later use.
/// @dev this contract is deployed in the L2GenesisUpgrade, and is meant as direct deployment without a proxy.
/// @param _l1ChainId The L1 chain id differs between mainnet and testnets.
/// @param _l2TokenProxyBytecodeHash The bytecode hash of the proxy for tokens deployed by the bridge.
/// @param _aliasedOwner The address of the governor contract.
Expand Down Expand Up @@ -92,33 +93,32 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault {
}

/// @notice Ensures that the token is deployed.
/// @param _originChainId The chain ID of the origin chain.
/// @param _assetId The asset ID.
/// @param _originToken The origin token address.
/// @param _erc20Data The ERC20 data.
/// @return expectedToken The token address.
function _ensureTokenDeployed(
uint256 _originChainId,
function _ensureAndSaveTokenDeployed(
bytes32 _assetId,
address _originToken,
bytes memory _erc20Data
) internal override returns (address expectedToken) {
expectedToken = _assetIdCheck(_originChainId, _assetId, _originToken);
uint256 tokenOriginChainId;
(expectedToken, tokenOriginChainId) = _calculateExpectedTokenAddress(_originToken, _erc20Data);
address l1LegacyToken;
if (address(L2_LEGACY_SHARED_BRIDGE) != address(0)) {
l1LegacyToken = L2_LEGACY_SHARED_BRIDGE.l1TokenAddress(expectedToken);
}

if (l1LegacyToken != address(0)) {
/// token is a legacy token, no need to deploy
if (l1LegacyToken != _originToken) {
revert AddressMismatch(_originToken, l1LegacyToken);
}
tokenAddress[_assetId] = expectedToken;
assetId[expectedToken] = _assetId;
_ensureAndSaveTokenDeployedInnerLegacyToken({
_assetId: _assetId,
_originToken: _originToken,
_expectedToken: expectedToken,
_l1LegacyToken: l1LegacyToken
});
} else {
super._ensureTokenDeployedInner({
_originChainId: _originChainId,
super._ensureAndSaveTokenDeployedInner({
_tokenOriginChainId: tokenOriginChainId,
_assetId: _assetId,
_originToken: _originToken,
_erc20Data: _erc20Data,
Expand All @@ -127,13 +127,35 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault {
}
}

/// @notice Deploys the beacon proxy for the L2 token, while using ContractDeployer system contract.
/// @notice Ensures that the token is deployed inner for legacy tokens.
function _ensureAndSaveTokenDeployedInnerLegacyToken(
bytes32 _assetId,
address _originToken,
address _expectedToken,
address _l1LegacyToken
) internal {
_assetIdCheck(L1_CHAIN_ID, _assetId, _originToken);

/// token is a legacy token, no need to deploy
if (_l1LegacyToken != _originToken) {
revert AddressMismatch(_originToken, _l1LegacyToken);
}

tokenAddress[_assetId] = _expectedToken;
assetId[_expectedToken] = _assetId;
}

/// @notice Deploys the beacon proxy for the L2 token, while using ContractDeployer system contract or the legacy shared bridge.
/// @dev This function uses raw call to ContractDeployer to make sure that exactly `l2TokenProxyBytecodeHash` is used
/// for the code of the proxy.
/// @param _salt The salt used for beacon proxy deployment of L2 bridged token.
/// @param _tokenOriginChainId The origin chain id of the token.
/// @return proxy The beacon proxy, i.e. L2 bridged token.
function _deployBeaconProxy(bytes32 _salt) internal virtual override returns (BeaconProxy proxy) {
if (address(L2_LEGACY_SHARED_BRIDGE) == address(0)) {
function _deployBeaconProxy(
bytes32 _salt,
uint256 _tokenOriginChainId
) internal virtual override returns (BeaconProxy proxy) {
if (address(L2_LEGACY_SHARED_BRIDGE) == address(0) || _tokenOriginChainId != L1_CHAIN_ID) {
// Deploy the beacon proxy for the L2 token

(bool success, bytes memory returndata) = SystemContractsCaller.systemCallWithReturndata(
Expand Down Expand Up @@ -172,17 +194,18 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault {
//////////////////////////////////////////////////////////////*/

/// @notice Calculates L2 wrapped token address given the currently stored beacon proxy bytecode hash and beacon address.
/// @param _tokenOriginChainId The chain id of the origin token.
/// @param _l1Token The address of token on L1.
/// @return Address of an L2 token counterpart.
function calculateCreate2TokenAddress(
uint256 _originChainId,
uint256 _tokenOriginChainId,
address _l1Token
) public view virtual override(INativeTokenVault, NativeTokenVault) returns (address) {
bytes32 constructorInputHash = keccak256(abi.encode(address(bridgedTokenBeacon), ""));
bytes32 salt = _getCreate2Salt(_originChainId, _l1Token);
if (address(L2_LEGACY_SHARED_BRIDGE) != address(0)) {
if (address(L2_LEGACY_SHARED_BRIDGE) != address(0) && _tokenOriginChainId == L1_CHAIN_ID) {
return L2_LEGACY_SHARED_BRIDGE.l2TokenAddress(_l1Token);
} else {
bytes32 constructorInputHash = keccak256(abi.encode(address(bridgedTokenBeacon), ""));
bytes32 salt = _getCreate2Salt(_tokenOriginChainId, _l1Token);
return
L2ContractHelper.computeCreate2Address(
address(this),
Expand All @@ -194,10 +217,13 @@ contract L2NativeTokenVault is IL2NativeTokenVault, NativeTokenVault {
}

/// @notice Calculates the salt for the Create2 deployment of the L2 token.
function _getCreate2Salt(uint256 _originChainId, address _l1Token) internal view override returns (bytes32 salt) {
salt = _originChainId == L1_CHAIN_ID
function _getCreate2Salt(
uint256 _tokenOriginChainId,
address _l1Token
) internal view override returns (bytes32 salt) {
salt = _tokenOriginChainId == L1_CHAIN_ID
? bytes32(uint256(uint160(_l1Token)))
: keccak256(abi.encode(_originChainId, _l1Token));
: keccak256(abi.encode(_tokenOriginChainId, _l1Token));
}

function _handleChainBalanceIncrease(
Expand Down
Loading

0 comments on commit 7198b54

Please sign in to comment.