From 18d9720dc2136bb27df4787e260113271e1b132c Mon Sep 17 00:00:00 2001 From: zkbenny Date: Fri, 19 Jan 2024 22:06:29 +0800 Subject: [PATCH 1/2] add data hash for withdraw --- contracts/Storage.sol | 11 ++++ contracts/ZkLink.sol | 17 ++++-- contracts/ZkLinkPeriphery.sol | 72 ++++++++++++++++++-------- contracts/dev-contracts/ZkLinkTest.sol | 2 +- contracts/zksync/Config.sol | 2 +- contracts/zksync/Events.sol | 3 ++ contracts/zksync/Operations.sol | 4 +- 7 files changed, 82 insertions(+), 29 deletions(-) diff --git a/contracts/Storage.sol b/contracts/Storage.sol index f0f9fae..61aaad4 100644 --- a/contracts/Storage.sol +++ b/contracts/Storage.sol @@ -125,6 +125,11 @@ contract Storage is ZkLinkAcceptor, Config { /// @dev Oracle verifier IOracleVerifier public oracleVerifier; + /// @dev Store withdraw data hash that need to be called + /// @dev The key is the withdraw data hash + /// @dev The value is a flag to indicating whether withdraw exists + mapping(bytes32 => bool) public pendingWithdrawWithCalls; + // #if CHAIN_ID == MASTER_CHAIN_ID /// @notice block stored data /// @dev `blockNumber`,`timestamp`,`stateHash`,`commitment` are the same on all chains @@ -233,6 +238,12 @@ contract Storage is ZkLinkAcceptor, Config { return _amount / SafeCast.toUint128(10**(TOKEN_DECIMALS_OF_LAYER2 - _decimals)); } + /// @dev Return withdraw hash with call data + /// @dev (_accountIdOfNonce, _subAccountIdOfNonce, _nonce) ensures the uniqueness of withdraw hash + function getWithdrawWithDataHash(address _owner, address _tokenAddress, uint128 _amount, bytes32 _dataHash, uint32 _accountIdOfNonce, uint8 _subAccountIdOfNonce, uint32 _nonce) internal pure returns (bytes32) { + return keccak256(abi.encodePacked(_owner, _tokenAddress, _amount, _dataHash, _accountIdOfNonce, _subAccountIdOfNonce, _nonce)); + } + /// @notice Performs a delegatecall to the contract implementation /// @dev Fallback function allowing to perform a delegatecall to the given implementation /// This function will return whatever the implementation call returns diff --git a/contracts/ZkLink.sol b/contracts/ZkLink.sol index 1d3b908..6bb4ba4 100644 --- a/contracts/ZkLink.sol +++ b/contracts/ZkLink.sol @@ -780,12 +780,12 @@ contract ZkLink is ReentrancyGuard, Storage, Events, UpgradeableMaster { if (opType == Operations.OpType.Withdraw) { Operations.Withdraw memory op = Operations.readWithdrawPubdata(pubData); // account request fast withdraw and sub account supply nonce - _executeWithdraw(op.accountId, op.subAccountId, op.nonce, op.owner, op.tokenId, op.amount, op.fastWithdrawFeeRate, op.withdrawToL1); + _executeWithdraw(op.accountId, op.subAccountId, op.nonce, op.owner, op.tokenId, op.amount, op.dataHash, op.fastWithdrawFeeRate, op.withdrawToL1); } else if (opType == Operations.OpType.ForcedExit) { Operations.ForcedExit memory op = Operations.readForcedExitPubdata(pubData); // request forced exit for target account but initiator sub account supply nonce - // forced exit require fast withdraw default and take no fee for fast withdraw - _executeWithdraw(op.initiatorAccountId, op.initiatorSubAccountId, op.initiatorNonce, op.target, op.tokenId, op.amount, 0, op.withdrawToL1); + // forced exit take no fee for fast withdraw + _executeWithdraw(op.initiatorAccountId, op.initiatorSubAccountId, op.initiatorNonce, op.target, op.tokenId, op.amount, bytes32(0), 0, op.withdrawToL1); } else if (opType == Operations.OpType.FullExit) { Operations.FullExit memory op = Operations.readFullExitPubdata(pubData); increasePendingBalance(op.tokenId, op.owner, op.amount); @@ -798,7 +798,7 @@ contract ZkLink is ReentrancyGuard, Storage, Events, UpgradeableMaster { } /// @dev The circuit will check whether there is dust in the amount - function _executeWithdraw(uint32 accountIdOfNonce, uint8 subAccountIdOfNonce, uint32 nonce, address owner, uint16 tokenId, uint128 amount, uint16 fastWithdrawFeeRate, uint8 withdrawToL1) internal { + function _executeWithdraw(uint32 accountIdOfNonce, uint8 subAccountIdOfNonce, uint32 nonce, address owner, uint16 tokenId, uint128 amount, bytes32 dataHash, uint16 fastWithdrawFeeRate, uint8 withdrawToL1) internal { // token MUST be registered RegisteredToken storage rt = tokens[tokenId]; require(rt.registered, "o0"); @@ -815,7 +815,14 @@ contract ZkLink is ReentrancyGuard, Storage, Events, UpgradeableMaster { if (acceptor == address(0)) { // receiver act as an acceptor accepts[withdrawHash] = owner; - increasePendingBalance(tokenId, owner, amount); + if (dataHash != bytes32(0)) { + // record token ownership to pending withdraw with calls and waiting relayer to call `withdrawPendingBalanceWithCall` + bytes32 withdrawWithDataHash = getWithdrawWithDataHash(owner, rt.tokenAddress, recoverAmount, dataHash, accountIdOfNonce, subAccountIdOfNonce, nonce); + pendingWithdrawWithCalls[withdrawWithDataHash] = true; + } else { + // record token ownership to pending balances and waiting relayer to call `withdrawPendingBalance` + increasePendingBalance(tokenId, owner, amount); + } } else { increasePendingBalance(tokenId, acceptor, amount); } diff --git a/contracts/ZkLinkPeriphery.sol b/contracts/ZkLinkPeriphery.sol index e9b63c5..ad8eb11 100644 --- a/contracts/ZkLinkPeriphery.sol +++ b/contracts/ZkLinkPeriphery.sol @@ -120,39 +120,69 @@ contract ZkLinkPeriphery is ReentrancyGuard, Storage, Events { /// @notice Withdraws tokens from zkLink contract to the owner /// @param _owner Address of the tokens owner /// @param _tokenId Token id - /// @param _amount Amount to withdraw to request. - /// @return The actual withdrawn amount - /// @dev NOTE: We will call ERC20.transfer(.., _amount), but if according to internal logic of ERC20 token zkLink contract - /// balance will be decreased by value more then _amount we will try to subtract this value from user pending balance - function withdrawPendingBalance(address payable _owner, uint16 _tokenId, uint128 _amount) external nonReentrant returns (uint128) { + /// @param _amount The actual withdrawn amount + function withdrawPendingBalance(address payable _owner, uint16 _tokenId, uint128 _amount) external nonReentrant { // ===Checks=== // token MUST be registered to ZkLink RegisteredToken storage rt = tokens[_tokenId]; require(rt.registered, "b0"); + // ===Effects=== // Set the available amount to withdraw // balance need to be recovery decimals - bytes32 owner = extendAddress(_owner); - uint128 balance = pendingBalances[owner][_tokenId]; - uint128 withdrawBalance = recoveryDecimals(balance, rt.decimals); - uint128 amount = Utils.minU128(withdrawBalance, _amount); - require(amount > 0, "b1"); + bytes32 l2Owner = extendAddress(_owner); + uint128 l2Balance = pendingBalances[l2Owner][_tokenId]; + uint128 l2Amount = improveDecimals(_amount, rt.decimals); + pendingBalances[l2Owner][_tokenId] = l2Balance - l2Amount; // ===Interactions=== - address tokenAddress = rt.tokenAddress; - if (tokenAddress == ETH_ADDRESS) { - // solhint-disable-next-line avoid-low-level-calls - (bool success, ) = _owner.call{value: amount}(""); - require(success, "b2"); + _withdrawTo(_owner, rt.tokenAddress, _amount, new bytes(0)); + emit Withdrawal(_tokenId, _amount); + } + + /// @notice Withdraws tokens from zkLink contract to the target contract + /// @param _owner Address of the tokens owner + /// @param _tokenAddress Token address + /// @param _amount The actual withdrawn amount + /// @param _data The target contract address and call data + /// @param _accountIdOfNonce Account that supply nonce + /// @param _subAccountIdOfNonce SubAccount that supply nonce + /// @param _nonce SubAccount nonce + /// @param _callTarget True when call target or withdraw pending balance to owner + function withdrawPendingBalanceWithCall(address payable _owner, address _tokenAddress, uint128 _amount, bytes memory _data, uint32 _accountIdOfNonce, uint8 _subAccountIdOfNonce, uint32 _nonce, bool _callTarget) external nonReentrant { + // ===Checks=== + // pending withdraw record MUST be exist + bytes32 dataHash = keccak256(_data); + bytes32 withdrawWithDataHash = getWithdrawWithDataHash(_owner, _tokenAddress, _amount, dataHash, _accountIdOfNonce, _subAccountIdOfNonce, _nonce); + require(pendingWithdrawWithCalls[withdrawWithDataHash], "z0"); + + // ===Effects=== + pendingWithdrawWithCalls[withdrawWithDataHash] = false; + + // decode data + (address targetContract, bytes memory callData) = abi.decode(_data, (address, bytes)); + if (_callTarget) { + _withdrawTo(payable(targetContract), _tokenAddress, _amount, callData); } else { - IERC20(tokenAddress).safeTransfer(_owner, amount); + _withdrawTo(_owner, _tokenAddress, _amount, new bytes(0)); } + emit WithdrawalCall(withdrawWithDataHash); + } - // improve withdrawn amount decimals - pendingBalances[owner][_tokenId] = balance - improveDecimals(amount, rt.decimals); - emit Withdrawal(_tokenId, amount); - - return amount; + function _withdrawTo(address payable _to, address _tokenAddress, uint128 _amount, bytes memory _callData) internal { + // ===Interactions=== + if (_tokenAddress == ETH_ADDRESS) { + // solhint-disable-next-line avoid-low-level-calls + (bool success, ) = _to.call{value: _amount}(_callData); + require(success, "b1"); + } else { + IERC20(_tokenAddress).safeTransfer(_to, _amount); + if (_callData.length > 0) { + // solhint-disable-next-line avoid-low-level-calls + (bool success, ) = _to.call(_callData); + require(success, "b2"); + } + } } /// @notice Returns amount of tokens that can be withdrawn by `address` from zkLink contract diff --git a/contracts/dev-contracts/ZkLinkTest.sol b/contracts/dev-contracts/ZkLinkTest.sol index 1f83489..40d281a 100644 --- a/contracts/dev-contracts/ZkLinkTest.sol +++ b/contracts/dev-contracts/ZkLinkTest.sol @@ -65,7 +65,7 @@ contract ZkLinkTest is ZkLink { // #endif function testExecuteWithdraw(Operations.Withdraw memory op) external { - _executeWithdraw(op.accountId, op.subAccountId, op.nonce, op.owner, op.tokenId, op.amount, op.fastWithdrawFeeRate, op.withdrawToL1); + _executeWithdraw(op.accountId, op.subAccountId, op.nonce, op.owner, op.tokenId, op.amount, op.dataHash, op.fastWithdrawFeeRate, op.withdrawToL1); } function testVerifyChangePubkey(bytes memory _ethWitness, Operations.ChangePubKey memory _changePk) external pure returns (bool) { diff --git a/contracts/zksync/Config.sol b/contracts/zksync/Config.sol index a54f978..f581fc7 100644 --- a/contracts/zksync/Config.sol +++ b/contracts/zksync/Config.sol @@ -31,7 +31,7 @@ contract Config { /// @dev Operation chunks uint256 internal constant DEPOSIT_BYTES = 3 * CHUNK_BYTES; uint256 internal constant FULL_EXIT_BYTES = 3 * CHUNK_BYTES; - uint256 internal constant WITHDRAW_BYTES = 3 * CHUNK_BYTES; + uint256 internal constant WITHDRAW_BYTES = 5 * CHUNK_BYTES; uint256 internal constant FORCED_EXIT_BYTES = 3 * CHUNK_BYTES; uint256 internal constant CHANGE_PUBKEY_BYTES = 3 * CHUNK_BYTES; diff --git a/contracts/zksync/Events.sol b/contracts/zksync/Events.sol index e974a3f..37537da 100644 --- a/contracts/zksync/Events.sol +++ b/contracts/zksync/Events.sol @@ -26,6 +26,9 @@ interface Events { /// @notice Event emitted when user funds are withdrawn from the zkLink state but not from contract event WithdrawalPending(uint16 indexed tokenId, bytes32 indexed recepient, uint128 amount); + /// @notice Event emitted when user funds are withdrawn from the zkLink state to a target contract + event WithdrawalCall(bytes32 indexed withdrawHash); + /// @notice Event emitted when user funds are withdrawn from the zkLink state to L1 and contract event WithdrawalL1(bytes32 indexed withdrawHash); diff --git a/contracts/zksync/Operations.sol b/contracts/zksync/Operations.sol index ecd0fd5..d762991 100644 --- a/contracts/zksync/Operations.sol +++ b/contracts/zksync/Operations.sol @@ -166,13 +166,14 @@ library Operations { uint16 tokenId; // the token that to withdraw //uint16 srcTokenId; -- the token that decreased in L2, present in pubdata, ignored at serialization uint128 amount; // the token amount to withdraw + bytes32 dataHash; // the call data for withdraw //uint16 fee; -- present in pubdata, ignored at serialization //bytes12 addressPrefixZero; -- address bytes length in L2 is 32 address owner; // the address to receive token uint32 nonce; // the sub account nonce uint16 fastWithdrawFeeRate; // fast withdraw fee rate taken by acceptor uint8 withdrawToL1; // when this flag is 1, it means withdraw token to L1 - } // 68 bytes + } // 100 bytes function readWithdrawPubdata(bytes memory _data) internal pure returns (Withdraw memory parsed) { // NOTE: there is no check that variable sizes are same as constants (i.e. TOKEN_BYTES), fix if possible. @@ -183,6 +184,7 @@ library Operations { (offset, parsed.tokenId) = Bytes.readUInt16(_data, offset); offset += TOKEN_BYTES; (offset, parsed.amount) = Bytes.readUInt128(_data, offset); + (offset, parsed.dataHash) = Bytes.readBytes32(_data, offset); offset += FEE_BYTES; offset += ADDRESS_PREFIX_ZERO_BYTES; (offset, parsed.owner) = Bytes.readAddress(_data, offset); From 70c435e4333f1a29b795ee173fff070f976a4609 Mon Sep 17 00:00:00 2001 From: zkbenny Date: Sat, 20 Jan 2024 14:30:02 +0800 Subject: [PATCH 2/2] update --- contracts/ZkLinkPeriphery.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/ZkLinkPeriphery.sol b/contracts/ZkLinkPeriphery.sol index ad8eb11..a1f24e3 100644 --- a/contracts/ZkLinkPeriphery.sol +++ b/contracts/ZkLinkPeriphery.sol @@ -159,9 +159,9 @@ contract ZkLinkPeriphery is ReentrancyGuard, Storage, Events { // ===Effects=== pendingWithdrawWithCalls[withdrawWithDataHash] = false; - // decode data - (address targetContract, bytes memory callData) = abi.decode(_data, (address, bytes)); if (_callTarget) { + // decode data + (address targetContract, bytes memory callData) = abi.decode(_data, (address, bytes)); _withdrawTo(payable(targetContract), _tokenAddress, _amount, callData); } else { _withdrawTo(_owner, _tokenAddress, _amount, new bytes(0));