From 139b4c250db934414e059175b46196af1b55e4a6 Mon Sep 17 00:00:00 2001 From: Ihor Barenblat Date: Tue, 2 Jun 2020 22:50:38 +0300 Subject: [PATCH 1/5] draft of ERC20 withdraw guard --- contracts/contracts/ZkSync.sol | 34 +- contracts/contracts/test/ZKSyncUnitTest.sol | 36 -- .../test/ZkSyncProcessOpUnitTest.sol | 16 + .../test/ZkSyncSignatureUnitTest.sol | 16 + .../test/ZkSyncWithdrawalUnitTest.sol | 21 + contracts/test/unit_tests/zksync_test.ts | 555 +++++++++--------- 6 files changed, 359 insertions(+), 319 deletions(-) delete mode 100644 contracts/contracts/test/ZKSyncUnitTest.sol create mode 100644 contracts/contracts/test/ZkSyncProcessOpUnitTest.sol create mode 100644 contracts/contracts/test/ZkSyncSignatureUnitTest.sol create mode 100644 contracts/contracts/test/ZkSyncWithdrawalUnitTest.sol diff --git a/contracts/contracts/ZkSync.sol b/contracts/contracts/ZkSync.sol index 50b7a76e4d..2dd4e00ce4 100644 --- a/contracts/contracts/ZkSync.sol +++ b/contracts/contracts/ZkSync.sol @@ -92,12 +92,29 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { return callSuccess && returnedSuccess; } + /// @notice Sends tokens + /// @dev NOTE: will revert if transfer call fails or rollup balance difference (before and after transfer) is bigger than _maxAmount + /// @param _token Token address + /// @param _to Address of recipient + /// @param _amount Amount of tokens to transfer + /// @param _maxAmount Maximum possible amount of tokens to transfer to this account + function withdrawERC20Guarded(IERC20 _token, address _to, uint128 _amount, uint128 _maxAmount) external returns (uint128 withdrawnAmount) { + require(msg.sender == address(this), "wtg10"); // wtg10 - can be called only from this contract as one "external" call (to revert all this function state changes if it is needed) + + uint256 balance_before = _token.balanceOf(address(this)); + require(sendERC20NoRevert(address(_token), _to, _amount), "wtg11"); // wtg11 - ERC20 transfer fails + uint256 balance_after = _token.balanceOf(address(this)); + require(balance_before.sub(balance_after) <= _maxAmount, "wtg12"); // wtg12 - rollup balance difference (before and after transfer) is bigger than _maxAmount + + return SafeCast.toUint128(balance_before.sub(balance_after)); + } + /// @notice Sends ETH /// @param _to Address of recipient /// @param _amount Amount of tokens to transfer /// @return bool flag indicating that transfer is successful function sendETHNoRevert(address payable _to, uint256 _amount) internal returns (bool) { - (bool callSuccess,) = _to.call.gas(ETH_WITHDRAWAL_GAS_LIMIT).value(_amount)(""); + (bool callSuccess, ) = _to.call.gas(ETH_WITHDRAWAL_GAS_LIMIT).value(_amount)(""); return callSuccess; } @@ -131,8 +148,10 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { sent = sendETHNoRevert(toPayable, amount); } else { address tokenAddr = governance.tokenAddresses(tokenId); - require(tokenAddr != address(0), "cwd11"); // unknown tokenId - sent = sendERC20NoRevert(tokenAddr, to, amount); + // we can just check that call not reverts because it wants to withdraw all amount + (sent,) = address(this).call.gas(ETH_WITHDRAWAL_GAS_LIMIT)( + abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128))", tokenAddr, to, amount, amount) + ); } if (!sent) { balancesToWithdraw[packedBalanceKey].balanceToWithdraw += amount; @@ -209,8 +228,13 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { /// @param _amount amount to withdraw function withdrawERC20(IERC20 _token, uint128 _amount) external nonReentrant { uint16 tokenId = governance.validateTokenAddress(address(_token)); - registerSingleWithdrawal(tokenId, _amount); - require(_token.transfer(msg.sender, _amount), "fw011"); // token transfer failed withdraw + bytes22 packedBalanceKey = packAddressAndTokenId(msg.sender, tokenId); + uint128 balance = balancesToWithdraw[packedBalanceKey].balanceToWithdraw; + (bool sent, bytes memory withdrawnAmountEncoded) = address(this).call( + abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128))", _token, msg.sender, _amount, balance) + ); + require(sent, "wt20"); // wt20 - withdrawERC20Guarded call fails + registerSingleWithdrawal(tokenId, abi.decode(withdrawnAmountEncoded, (uint128))); } /// @notice Register full exit request - pack pubdata, add priority request diff --git a/contracts/contracts/test/ZKSyncUnitTest.sol b/contracts/contracts/test/ZKSyncUnitTest.sol deleted file mode 100644 index 917d468929..0000000000 --- a/contracts/contracts/test/ZKSyncUnitTest.sol +++ /dev/null @@ -1,36 +0,0 @@ -pragma solidity ^0.5.0; - -import "../generated/ZkSyncTest.sol"; - - -contract ZKSyncUnitTest is ZkSyncTest { - - function changePubkeySignatureCheck(bytes calldata _signature, bytes20 _newPkHash, uint32 _nonce, address _ethAddress, uint24 _accountId) external pure returns (bool) { - return verifyChangePubkeySignature(_signature, _newPkHash, _nonce, _ethAddress, _accountId); - } - - function setBalanceToWithdraw(address _owner, uint16 _token, uint128 _amount) external { - balancesToWithdraw[packAddressAndTokenId(_owner, _token)].balanceToWithdraw = _amount; - } - - function receiveETH() payable external{} - - function addPendingWithdrawal(address _to, uint16 _tokenId, uint128 _amount) external { - pendingWithdrawals[firstPendingWithdrawalIndex + numberOfPendingWithdrawals] = PendingWithdrawal(_to, _tokenId); - numberOfPendingWithdrawals++; - bytes22 packedBalanceKey = packAddressAndTokenId(_to, _tokenId); - balancesToWithdraw[packedBalanceKey].balanceToWithdraw += _amount; - } - - function testProcessOperation( - bytes calldata _publicData, - bytes calldata _ethWitness, - uint32[] calldata _ethWitnessSizes - ) external { - collectOnchainOps(0, _publicData, _ethWitness, _ethWitnessSizes); - } - - function testRecoverAddressFromEthSignature(bytes calldata _signature, bytes calldata _message) external pure returns (address) { - return recoverAddressFromEthSignature(_signature, _message); - } -} diff --git a/contracts/contracts/test/ZkSyncProcessOpUnitTest.sol b/contracts/contracts/test/ZkSyncProcessOpUnitTest.sol new file mode 100644 index 0000000000..ded2d0bd0e --- /dev/null +++ b/contracts/contracts/test/ZkSyncProcessOpUnitTest.sol @@ -0,0 +1,16 @@ +pragma solidity ^0.5.0; + +import "../generated/ZkSyncTest.sol"; + + +contract ZkSyncProcessOpUnitTest is ZkSyncTest { + + function testProcessOperation( + bytes calldata _publicData, + bytes calldata _ethWitness, + uint32[] calldata _ethWitnessSizes + ) external { + collectOnchainOps(0, _publicData, _ethWitness, _ethWitnessSizes); + } + +} diff --git a/contracts/contracts/test/ZkSyncSignatureUnitTest.sol b/contracts/contracts/test/ZkSyncSignatureUnitTest.sol new file mode 100644 index 0000000000..3ce12c35b0 --- /dev/null +++ b/contracts/contracts/test/ZkSyncSignatureUnitTest.sol @@ -0,0 +1,16 @@ +pragma solidity ^0.5.0; + +import "../generated/ZkSyncTest.sol"; + + +contract ZKSyncSignatureUnitTest is ZkSyncTest { + + function changePubkeySignatureCheck(bytes calldata _signature, bytes20 _newPkHash, uint32 _nonce, address _ethAddress, uint24 _accountId) external pure returns (bool) { + return verifyChangePubkeySignature(_signature, _newPkHash, _nonce, _ethAddress, _accountId); + } + + function testRecoverAddressFromEthSignature(bytes calldata _signature, bytes calldata _message) external pure returns (address) { + return recoverAddressFromEthSignature(_signature, _message); + } + +} diff --git a/contracts/contracts/test/ZkSyncWithdrawalUnitTest.sol b/contracts/contracts/test/ZkSyncWithdrawalUnitTest.sol new file mode 100644 index 0000000000..67d3934dc5 --- /dev/null +++ b/contracts/contracts/test/ZkSyncWithdrawalUnitTest.sol @@ -0,0 +1,21 @@ +pragma solidity ^0.5.0; + +import "../generated/ZkSyncTest.sol"; + + +contract ZkSyncWithdrawalUnitTest is ZkSyncTest { + + function setBalanceToWithdraw(address _owner, uint16 _token, uint128 _amount) external { + balancesToWithdraw[packAddressAndTokenId(_owner, _token)].balanceToWithdraw = _amount; + } + + function receiveETH() payable external{} + + function addPendingWithdrawal(address _to, uint16 _tokenId, uint128 _amount) external { + pendingWithdrawals[firstPendingWithdrawalIndex + numberOfPendingWithdrawals] = PendingWithdrawal(_to, _tokenId); + numberOfPendingWithdrawals++; + bytes22 packedBalanceKey = packAddressAndTokenId(_to, _tokenId); + balancesToWithdraw[packedBalanceKey].balanceToWithdraw += _amount; + } + +} diff --git a/contracts/test/unit_tests/zksync_test.ts b/contracts/test/unit_tests/zksync_test.ts index 8f04bd7bfc..e2d3c51836 100644 --- a/contracts/test/unit_tests/zksync_test.ts +++ b/contracts/test/unit_tests/zksync_test.ts @@ -24,7 +24,7 @@ describe("zkSync signature verification unit tests", function() { const randomWallet = ethers.Wallet.createRandom(); before(async () => { const contracts = readTestContracts(); - contracts.zkSync = readContractCode("ZKSyncUnitTest"); + contracts.zkSync = readContractCode("ZKSyncSignatureUnitTest"); const deployer = new Deployer({deployWallet: wallet, contracts}); await deployer.deployAll({gasLimit: 6500000}); testContract = deployer.zkSyncContract(wallet); @@ -239,7 +239,7 @@ describe("zkSync withdraw unit tests", function() { let ethProxy; before(async () => { const contracts = readTestContracts(); - contracts.zkSync = readContractCode("ZKSyncUnitTest"); + contracts.zkSync = readContractCode("ZkSyncWithdrawalUnitTest"); const deployer = new Deployer({deployWallet: wallet, contracts}); await deployer.deployAll({gasLimit: 6500000}); zksyncContract = deployer.zkSyncContract(wallet); @@ -350,7 +350,7 @@ describe("zkSync withdraw unit tests", function() { await zksyncContract.setBalanceToWithdraw(wallet.address, tokenId, withdrawAmount); const {revertReason} = await getCallRevertReason(async () => await performWithdraw(wallet, tokenContract.address, tokenId, withdrawAmount.add(1))); - expect(revertReason, "wrong revert reason").eq("SafeMath: subtraction overflow"); + expect(revertReason, "wrong revert reason").eq("wt20"); }); it("Withdraw ERC20 unsupported token", async () => { @@ -391,278 +391,277 @@ describe("zkSync withdraw unit tests", function() { }); }); -describe("zkSync auth pubkey onchain unit tests", function() { - this.timeout(50000); - - let zksyncContract; - let tokenContract; - let ethProxy; - before(async () => { - const contracts = readTestContracts(); - contracts.zkSync = readContractCode("ZKSyncUnitTest"); - const deployer = new Deployer({deployWallet: wallet, contracts}); - await deployer.deployAll({gasLimit: 6500000}); - zksyncContract = deployer.zkSyncContract(wallet); - - tokenContract = await deployContract( - wallet, - readContractCode("TEST-ERC20"), [], - {gasLimit: 5000000}, - ); - await tokenContract.mint(wallet.address, parseEther("1000000")); - - const govContract = deployer.governanceContract(wallet); - await govContract.addToken(tokenContract.address); - - ethProxy = new ETHProxy(wallet.provider, { - mainContract: zksyncContract.address, - govContract: govContract.address, - }); - }); - - it("Auth pubkey success", async () => { - zksyncContract.connect(wallet); - - const nonce = 0x1234; - const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; - - const receipt = await (await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce)).wait(); - let authEvent; - for (const event of receipt.logs) { - const parsedLog = zksyncContract.interface.parseLog(event); - if (parsedLog && parsedLog.name === "FactAuth") { - authEvent = parsedLog; - break; - } - } - - expect(authEvent.values.sender, "event sender incorrect").eq(wallet.address); - expect(authEvent.values.nonce, "event nonce incorrect").eq(nonce); - expect(authEvent.values.fact, "event fact incorrect").eq(pubkeyHash); - }); - - it("Auth pubkey rewrite fail", async () => { - zksyncContract.connect(wallet); - - const nonce = 0xdead; - const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; - - await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); - // - const otherPubkeyHash = "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; - const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(otherPubkeyHash, nonce)); - expect(revertReason, "revert reason incorrect").eq("ahf11"); - }); - - it("Auth pubkey incorrect length fail", async () => { - zksyncContract.connect(wallet); - const nonce = 0x7656; - const shortPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefe"; - const longPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefefe"; - - for (const pkHash of [shortPubkeyHash, longPubkeyHash]) { - const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(shortPubkeyHash, nonce)); - expect(revertReason, "revert reason incorrect").eq("ahf10"); - } - }); -}); - -describe("zkSync test process next operation", function() { - this.timeout(50000); - - let zksyncContract; - let tokenContract; - let incorrectTokenContract; - let ethProxy; - before(async () => { - const contracts = readTestContracts(); - contracts.zkSync = readContractCode("ZKSyncUnitTest"); - const deployer = new Deployer({deployWallet: wallet, contracts}); - await deployer.deployAll({gasLimit: 6500000}); - zksyncContract = deployer.zkSyncContract(wallet); - - tokenContract = await deployContract( - wallet, - readContractCode("TEST-ERC20"), [], - {gasLimit: 5000000}, - ); - await tokenContract.mint(wallet.address, parseEther("1000000")); - - const govContract = deployer.governanceContract(wallet); - await govContract.addToken(tokenContract.address); - - ethProxy = new ETHProxy(wallet.provider, { - mainContract: zksyncContract.address, - govContract: govContract.address - }); - - incorrectTokenContract = await deployContract( - wallet, - readContractCode("TEST-ERC20"), [], - {gasLimit: 5000000}, - ); - await tokenContract.mint(wallet.address, parseEther("1000000")); - }); - - it("Process noop", async () => { - zksyncContract.connect(wallet); - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - const pubdata = Buffer.alloc(CHUNK_SIZE, 0); - await zksyncContract.testProcessOperation(pubdata, "0x", []); - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); - }); - - it("Process transfer", async () => { - zksyncContract.connect(wallet); - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - const pubdata = Buffer.alloc(CHUNK_SIZE * 2, 0xff); - pubdata[0] = 0x05; - await zksyncContract.testProcessOperation(pubdata, "0x", []); - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); - }); - it("Process transfer to new", async () => { - zksyncContract.connect(wallet); - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0xff); - pubdata[0] = 0x02; - await zksyncContract.testProcessOperation(pubdata, "0x", []); - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); - }); - - it("Process deposit", async () => { - zksyncContract.connect(wallet); - const depositAmount = bigNumberify("2"); - - await zksyncContract.depositETH(wallet.address, {value: depositAmount}); - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - // construct deposit pubdata - const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); - pubdata[0] = 0x01; - let offset = 1; - pubdata.writeUInt32BE(0xccaabbff, offset); - offset += 4; - pubdata.writeUInt16BE(0, offset); // token - offset += 2; - Buffer.from(depositAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); - offset += 16; - Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); - await zksyncContract.testProcessOperation(pubdata, "0x", []); - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); - }); - - it("Process partial exit", async () => { - zksyncContract.connect(wallet); - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - // construct deposit pubdata - const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); - pubdata[0] = 0x03; - - await zksyncContract.testProcessOperation(pubdata, "0x", []); - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); - }); - - it("Process full exit", async () => { - zksyncContract.connect(wallet); - const tokenId = await ethProxy.resolveTokenId(tokenContract.address); - const fullExitAmount = parseEther("0.7"); - const accountId = 0xaabbffcc; - - await zksyncContract.fullExit(accountId, tokenContract.address); - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - // construct full exit pubdata - const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); - pubdata[0] = 0x06; - let offset = 1; - pubdata.writeUInt32BE(accountId, offset); - offset += 4; - Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); - offset += 20; - pubdata.writeUInt16BE(tokenId, offset); - offset += 2; - Buffer.from(fullExitAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); - - await zksyncContract.testProcessOperation(pubdata, "0x", []); - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); - }); - - it("Change pubkey with auth", async () => { - zksyncContract.connect(wallet); - - const nonce = 0x1234; - const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; - await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); - - const accountId = 0xffee12cc; - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - // construct deposit pubdata - const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); - pubdata[0] = 0x07; - let offset = 1; - pubdata.writeUInt32BE(accountId, offset); - offset += 4; - Buffer.from(pubkeyHash.substr(2), "hex").copy(pubdata, offset); - offset += 20; - Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); - offset += 20; - pubdata.writeUInt32BE(nonce, offset); - - await zksyncContract.testProcessOperation(pubdata, "0x", [0]); - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); - }); - - it("Change pubkey with posted signature", async () => { - zksyncContract.connect(wallet); - - const nonce = 0x1234; - const pubkeyHash = "sync:fefefefefefefefefefefefefefefefefefefefe"; - const accountId = 0x00ffee12; - const ethWitness = await zksync.utils.signChangePubkeyMessage(wallet, pubkeyHash, nonce, accountId); - - const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); - - // construct deposit pubdata - const pubdata = Buffer.alloc( CHUNK_SIZE * 6, 0); - pubdata[0] = 0x07; - let offset = 1; - pubdata.writeUInt32BE(accountId, offset); - offset += 4; - Buffer.from(pubkeyHash.substr(5), "hex").copy(pubdata, offset); - offset += 20; - Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); - offset += 20; - pubdata.writeUInt32BE(nonce, offset); - - await zksyncContract.testProcessOperation(pubdata, ethWitness, [(ethWitness.length - 2) / 2]); // (ethWitness.length - 2) / 2 == len of ethWitness in bytes - - const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); - expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); - }); -}); +// describe("zkSync auth pubkey onchain unit tests", function() { +// this.timeout(50000); +// +// let zksyncContract; +// let tokenContract; +// let ethProxy; +// before(async () => { +// const contracts = readTestContracts(); +// const deployer = new Deployer({deployWallet: wallet, contracts}); +// await deployer.deployAll({gasLimit: 6500000}); +// zksyncContract = deployer.zkSyncContract(wallet); +// +// tokenContract = await deployContract( +// wallet, +// readContractCode("TEST-ERC20"), [], +// {gasLimit: 5000000}, +// ); +// await tokenContract.mint(wallet.address, parseEther("1000000")); +// +// const govContract = deployer.governanceContract(wallet); +// await govContract.addToken(tokenContract.address); +// +// ethProxy = new ETHProxy(wallet.provider, { +// mainContract: zksyncContract.address, +// govContract: govContract.address, +// }); +// }); +// +// it("Auth pubkey success", async () => { +// zksyncContract.connect(wallet); +// +// const nonce = 0x1234; +// const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; +// +// const receipt = await (await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce)).wait(); +// let authEvent; +// for (const event of receipt.logs) { +// const parsedLog = zksyncContract.interface.parseLog(event); +// if (parsedLog && parsedLog.name === "FactAuth") { +// authEvent = parsedLog; +// break; +// } +// } +// +// expect(authEvent.values.sender, "event sender incorrect").eq(wallet.address); +// expect(authEvent.values.nonce, "event nonce incorrect").eq(nonce); +// expect(authEvent.values.fact, "event fact incorrect").eq(pubkeyHash); +// }); +// +// it("Auth pubkey rewrite fail", async () => { +// zksyncContract.connect(wallet); +// +// const nonce = 0xdead; +// const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; +// +// await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); +// // +// const otherPubkeyHash = "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +// const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(otherPubkeyHash, nonce)); +// expect(revertReason, "revert reason incorrect").eq("ahf11"); +// }); +// +// it("Auth pubkey incorrect length fail", async () => { +// zksyncContract.connect(wallet); +// const nonce = 0x7656; +// const shortPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefe"; +// const longPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefefe"; +// +// for (const pkHash of [shortPubkeyHash, longPubkeyHash]) { +// const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(shortPubkeyHash, nonce)); +// expect(revertReason, "revert reason incorrect").eq("ahf10"); +// } +// }); +// }); + +// describe("zkSync test process next operation", function() { +// this.timeout(50000); +// +// let zksyncContract; +// let tokenContract; +// let incorrectTokenContract; +// let ethProxy; +// before(async () => { +// const contracts = readTestContracts(); +// contracts.zkSync = readContractCode("ZkSyncProcessOpUnitTest"); +// const deployer = new Deployer({deployWallet: wallet, contracts}); +// await deployer.deployAll({gasLimit: 6500000}); +// zksyncContract = deployer.zkSyncContract(wallet); +// +// tokenContract = await deployContract( +// wallet, +// readContractCode("TEST-ERC20"), [], +// {gasLimit: 5000000}, +// ); +// await tokenContract.mint(wallet.address, parseEther("1000000")); +// +// const govContract = deployer.governanceContract(wallet); +// await govContract.addToken(tokenContract.address); +// +// ethProxy = new ETHProxy(wallet.provider, { +// mainContract: zksyncContract.address, +// govContract: govContract.address +// }); +// +// incorrectTokenContract = await deployContract( +// wallet, +// readContractCode("TEST-ERC20"), [], +// {gasLimit: 5000000}, +// ); +// await tokenContract.mint(wallet.address, parseEther("1000000")); +// }); +// +// it("Process noop", async () => { +// zksyncContract.connect(wallet); +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// const pubdata = Buffer.alloc(CHUNK_SIZE, 0); +// await zksyncContract.testProcessOperation(pubdata, "0x", []); +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// +// it("Process transfer", async () => { +// zksyncContract.connect(wallet); +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// const pubdata = Buffer.alloc(CHUNK_SIZE * 2, 0xff); +// pubdata[0] = 0x05; +// await zksyncContract.testProcessOperation(pubdata, "0x", []); +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// it("Process transfer to new", async () => { +// zksyncContract.connect(wallet); +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0xff); +// pubdata[0] = 0x02; +// await zksyncContract.testProcessOperation(pubdata, "0x", []); +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// +// it("Process deposit", async () => { +// zksyncContract.connect(wallet); +// const depositAmount = bigNumberify("2"); +// +// await zksyncContract.depositETH(wallet.address, {value: depositAmount}); +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// // construct deposit pubdata +// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); +// pubdata[0] = 0x01; +// let offset = 1; +// pubdata.writeUInt32BE(0xccaabbff, offset); +// offset += 4; +// pubdata.writeUInt16BE(0, offset); // token +// offset += 2; +// Buffer.from(depositAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); +// offset += 16; +// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); +// await zksyncContract.testProcessOperation(pubdata, "0x", []); +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// +// it("Process partial exit", async () => { +// zksyncContract.connect(wallet); +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// // construct deposit pubdata +// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); +// pubdata[0] = 0x03; +// +// await zksyncContract.testProcessOperation(pubdata, "0x", []); +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// +// it("Process full exit", async () => { +// zksyncContract.connect(wallet); +// const tokenId = await ethProxy.resolveTokenId(tokenContract.address); +// const fullExitAmount = parseEther("0.7"); +// const accountId = 0xaabbffcc; +// +// await zksyncContract.fullExit(accountId, tokenContract.address); +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// // construct full exit pubdata +// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); +// pubdata[0] = 0x06; +// let offset = 1; +// pubdata.writeUInt32BE(accountId, offset); +// offset += 4; +// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); +// offset += 20; +// pubdata.writeUInt16BE(tokenId, offset); +// offset += 2; +// Buffer.from(fullExitAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); +// +// await zksyncContract.testProcessOperation(pubdata, "0x", []); +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// +// it("Change pubkey with auth", async () => { +// zksyncContract.connect(wallet); +// +// const nonce = 0x1234; +// const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; +// await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); +// +// const accountId = 0xffee12cc; +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// // construct deposit pubdata +// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); +// pubdata[0] = 0x07; +// let offset = 1; +// pubdata.writeUInt32BE(accountId, offset); +// offset += 4; +// Buffer.from(pubkeyHash.substr(2), "hex").copy(pubdata, offset); +// offset += 20; +// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); +// offset += 20; +// pubdata.writeUInt32BE(nonce, offset); +// +// await zksyncContract.testProcessOperation(pubdata, "0x", [0]); +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// +// it("Change pubkey with posted signature", async () => { +// zksyncContract.connect(wallet); +// +// const nonce = 0x1234; +// const pubkeyHash = "sync:fefefefefefefefefefefefefefefefefefefefe"; +// const accountId = 0x00ffee12; +// const ethWitness = await zksync.utils.signChangePubkeyMessage(wallet, pubkeyHash, nonce, accountId); +// +// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); +// +// // construct deposit pubdata +// const pubdata = Buffer.alloc( CHUNK_SIZE * 6, 0); +// pubdata[0] = 0x07; +// let offset = 1; +// pubdata.writeUInt32BE(accountId, offset); +// offset += 4; +// Buffer.from(pubkeyHash.substr(5), "hex").copy(pubdata, offset); +// offset += 20; +// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); +// offset += 20; +// pubdata.writeUInt32BE(nonce, offset); +// +// await zksyncContract.testProcessOperation(pubdata, ethWitness, [(ethWitness.length - 2) / 2]); // (ethWitness.length - 2) / 2 == len of ethWitness in bytes +// +// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); +// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); +// }); +// }); From 65148b7f9e78a3efd716a85d236e9ceaaced0076 Mon Sep 17 00:00:00 2001 From: Ihor Barenblat Date: Wed, 3 Jun 2020 13:35:36 +0300 Subject: [PATCH 2/5] fixed signature in withdrawERC20Guarded call --- contracts/contracts/ZkSync.sol | 12 +- contracts/test/unit_tests/zksync_test.ts | 548 +++++++++++------------ 2 files changed, 280 insertions(+), 280 deletions(-) diff --git a/contracts/contracts/ZkSync.sol b/contracts/contracts/ZkSync.sol index 2dd4e00ce4..30a51585f8 100644 --- a/contracts/contracts/ZkSync.sol +++ b/contracts/contracts/ZkSync.sol @@ -83,8 +83,8 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { /// @param _to Address of recipient /// @param _amount Amount of tokens to transfer /// @return bool flag indicating that transfer is successful - function sendERC20NoRevert(address _token, address _to, uint256 _amount) internal returns (bool) { - (bool callSuccess, bytes memory callReturnValueEncoded) = _token.call.gas(ERC20_WITHDRAWAL_GAS_LIMIT)( + function sendERC20(address _token, address _to, uint256 _amount) internal returns (bool) { + (bool callSuccess, bytes memory callReturnValueEncoded) = _token.call( abi.encodeWithSignature("transfer(address,uint256)", _to, _amount) ); // `transfer` method may return (bool) or nothing. @@ -102,7 +102,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { require(msg.sender == address(this), "wtg10"); // wtg10 - can be called only from this contract as one "external" call (to revert all this function state changes if it is needed) uint256 balance_before = _token.balanceOf(address(this)); - require(sendERC20NoRevert(address(_token), _to, _amount), "wtg11"); // wtg11 - ERC20 transfer fails + require(sendERC20(address(_token), _to, _amount), "wtg11"); // wtg11 - ERC20 transfer fails uint256 balance_after = _token.balanceOf(address(this)); require(balance_before.sub(balance_after) <= _maxAmount, "wtg12"); // wtg12 - rollup balance difference (before and after transfer) is bigger than _maxAmount @@ -149,8 +149,8 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { } else { address tokenAddr = governance.tokenAddresses(tokenId); // we can just check that call not reverts because it wants to withdraw all amount - (sent,) = address(this).call.gas(ETH_WITHDRAWAL_GAS_LIMIT)( - abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128))", tokenAddr, to, amount, amount) + (sent,) = address(this).call.gas(ERC20_WITHDRAWAL_GAS_LIMIT)( + abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128)", tokenAddr, to, amount, amount) ); } if (!sent) { @@ -231,7 +231,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { bytes22 packedBalanceKey = packAddressAndTokenId(msg.sender, tokenId); uint128 balance = balancesToWithdraw[packedBalanceKey].balanceToWithdraw; (bool sent, bytes memory withdrawnAmountEncoded) = address(this).call( - abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128))", _token, msg.sender, _amount, balance) + abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128)", _token, msg.sender, _amount, balance) ); require(sent, "wt20"); // wt20 - withdrawERC20Guarded call fails registerSingleWithdrawal(tokenId, abi.decode(withdrawnAmountEncoded, (uint128))); diff --git a/contracts/test/unit_tests/zksync_test.ts b/contracts/test/unit_tests/zksync_test.ts index e2d3c51836..05279c57e9 100644 --- a/contracts/test/unit_tests/zksync_test.ts +++ b/contracts/test/unit_tests/zksync_test.ts @@ -391,277 +391,277 @@ describe("zkSync withdraw unit tests", function() { }); }); -// describe("zkSync auth pubkey onchain unit tests", function() { -// this.timeout(50000); -// -// let zksyncContract; -// let tokenContract; -// let ethProxy; -// before(async () => { -// const contracts = readTestContracts(); -// const deployer = new Deployer({deployWallet: wallet, contracts}); -// await deployer.deployAll({gasLimit: 6500000}); -// zksyncContract = deployer.zkSyncContract(wallet); -// -// tokenContract = await deployContract( -// wallet, -// readContractCode("TEST-ERC20"), [], -// {gasLimit: 5000000}, -// ); -// await tokenContract.mint(wallet.address, parseEther("1000000")); -// -// const govContract = deployer.governanceContract(wallet); -// await govContract.addToken(tokenContract.address); -// -// ethProxy = new ETHProxy(wallet.provider, { -// mainContract: zksyncContract.address, -// govContract: govContract.address, -// }); -// }); -// -// it("Auth pubkey success", async () => { -// zksyncContract.connect(wallet); -// -// const nonce = 0x1234; -// const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; -// -// const receipt = await (await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce)).wait(); -// let authEvent; -// for (const event of receipt.logs) { -// const parsedLog = zksyncContract.interface.parseLog(event); -// if (parsedLog && parsedLog.name === "FactAuth") { -// authEvent = parsedLog; -// break; -// } -// } -// -// expect(authEvent.values.sender, "event sender incorrect").eq(wallet.address); -// expect(authEvent.values.nonce, "event nonce incorrect").eq(nonce); -// expect(authEvent.values.fact, "event fact incorrect").eq(pubkeyHash); -// }); -// -// it("Auth pubkey rewrite fail", async () => { -// zksyncContract.connect(wallet); -// -// const nonce = 0xdead; -// const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; -// -// await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); -// // -// const otherPubkeyHash = "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; -// const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(otherPubkeyHash, nonce)); -// expect(revertReason, "revert reason incorrect").eq("ahf11"); -// }); -// -// it("Auth pubkey incorrect length fail", async () => { -// zksyncContract.connect(wallet); -// const nonce = 0x7656; -// const shortPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefe"; -// const longPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefefe"; -// -// for (const pkHash of [shortPubkeyHash, longPubkeyHash]) { -// const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(shortPubkeyHash, nonce)); -// expect(revertReason, "revert reason incorrect").eq("ahf10"); -// } -// }); -// }); - -// describe("zkSync test process next operation", function() { -// this.timeout(50000); -// -// let zksyncContract; -// let tokenContract; -// let incorrectTokenContract; -// let ethProxy; -// before(async () => { -// const contracts = readTestContracts(); -// contracts.zkSync = readContractCode("ZkSyncProcessOpUnitTest"); -// const deployer = new Deployer({deployWallet: wallet, contracts}); -// await deployer.deployAll({gasLimit: 6500000}); -// zksyncContract = deployer.zkSyncContract(wallet); -// -// tokenContract = await deployContract( -// wallet, -// readContractCode("TEST-ERC20"), [], -// {gasLimit: 5000000}, -// ); -// await tokenContract.mint(wallet.address, parseEther("1000000")); -// -// const govContract = deployer.governanceContract(wallet); -// await govContract.addToken(tokenContract.address); -// -// ethProxy = new ETHProxy(wallet.provider, { -// mainContract: zksyncContract.address, -// govContract: govContract.address -// }); -// -// incorrectTokenContract = await deployContract( -// wallet, -// readContractCode("TEST-ERC20"), [], -// {gasLimit: 5000000}, -// ); -// await tokenContract.mint(wallet.address, parseEther("1000000")); -// }); -// -// it("Process noop", async () => { -// zksyncContract.connect(wallet); -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// const pubdata = Buffer.alloc(CHUNK_SIZE, 0); -// await zksyncContract.testProcessOperation(pubdata, "0x", []); -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// -// it("Process transfer", async () => { -// zksyncContract.connect(wallet); -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// const pubdata = Buffer.alloc(CHUNK_SIZE * 2, 0xff); -// pubdata[0] = 0x05; -// await zksyncContract.testProcessOperation(pubdata, "0x", []); -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// it("Process transfer to new", async () => { -// zksyncContract.connect(wallet); -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0xff); -// pubdata[0] = 0x02; -// await zksyncContract.testProcessOperation(pubdata, "0x", []); -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// -// it("Process deposit", async () => { -// zksyncContract.connect(wallet); -// const depositAmount = bigNumberify("2"); -// -// await zksyncContract.depositETH(wallet.address, {value: depositAmount}); -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// // construct deposit pubdata -// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); -// pubdata[0] = 0x01; -// let offset = 1; -// pubdata.writeUInt32BE(0xccaabbff, offset); -// offset += 4; -// pubdata.writeUInt16BE(0, offset); // token -// offset += 2; -// Buffer.from(depositAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); -// offset += 16; -// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); -// await zksyncContract.testProcessOperation(pubdata, "0x", []); -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// -// it("Process partial exit", async () => { -// zksyncContract.connect(wallet); -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// // construct deposit pubdata -// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); -// pubdata[0] = 0x03; -// -// await zksyncContract.testProcessOperation(pubdata, "0x", []); -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// -// it("Process full exit", async () => { -// zksyncContract.connect(wallet); -// const tokenId = await ethProxy.resolveTokenId(tokenContract.address); -// const fullExitAmount = parseEther("0.7"); -// const accountId = 0xaabbffcc; -// -// await zksyncContract.fullExit(accountId, tokenContract.address); -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// // construct full exit pubdata -// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); -// pubdata[0] = 0x06; -// let offset = 1; -// pubdata.writeUInt32BE(accountId, offset); -// offset += 4; -// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); -// offset += 20; -// pubdata.writeUInt16BE(tokenId, offset); -// offset += 2; -// Buffer.from(fullExitAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); -// -// await zksyncContract.testProcessOperation(pubdata, "0x", []); -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// -// it("Change pubkey with auth", async () => { -// zksyncContract.connect(wallet); -// -// const nonce = 0x1234; -// const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; -// await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); -// -// const accountId = 0xffee12cc; -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// // construct deposit pubdata -// const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); -// pubdata[0] = 0x07; -// let offset = 1; -// pubdata.writeUInt32BE(accountId, offset); -// offset += 4; -// Buffer.from(pubkeyHash.substr(2), "hex").copy(pubdata, offset); -// offset += 20; -// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); -// offset += 20; -// pubdata.writeUInt32BE(nonce, offset); -// -// await zksyncContract.testProcessOperation(pubdata, "0x", [0]); -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// -// it("Change pubkey with posted signature", async () => { -// zksyncContract.connect(wallet); -// -// const nonce = 0x1234; -// const pubkeyHash = "sync:fefefefefefefefefefefefefefefefefefefefe"; -// const accountId = 0x00ffee12; -// const ethWitness = await zksync.utils.signChangePubkeyMessage(wallet, pubkeyHash, nonce, accountId); -// -// const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); -// -// // construct deposit pubdata -// const pubdata = Buffer.alloc( CHUNK_SIZE * 6, 0); -// pubdata[0] = 0x07; -// let offset = 1; -// pubdata.writeUInt32BE(accountId, offset); -// offset += 4; -// Buffer.from(pubkeyHash.substr(5), "hex").copy(pubdata, offset); -// offset += 20; -// Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); -// offset += 20; -// pubdata.writeUInt32BE(nonce, offset); -// -// await zksyncContract.testProcessOperation(pubdata, ethWitness, [(ethWitness.length - 2) / 2]); // (ethWitness.length - 2) / 2 == len of ethWitness in bytes -// -// const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); -// expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); -// }); -// }); +describe("zkSync auth pubkey onchain unit tests", function() { + this.timeout(50000); + + let zksyncContract; + let tokenContract; + let ethProxy; + before(async () => { + const contracts = readTestContracts(); + const deployer = new Deployer({deployWallet: wallet, contracts}); + await deployer.deployAll({gasLimit: 6500000}); + zksyncContract = deployer.zkSyncContract(wallet); + + tokenContract = await deployContract( + wallet, + readContractCode("TEST-ERC20"), [], + {gasLimit: 5000000}, + ); + await tokenContract.mint(wallet.address, parseEther("1000000")); + + const govContract = deployer.governanceContract(wallet); + await govContract.addToken(tokenContract.address); + + ethProxy = new ETHProxy(wallet.provider, { + mainContract: zksyncContract.address, + govContract: govContract.address, + }); + }); + + it("Auth pubkey success", async () => { + zksyncContract.connect(wallet); + + const nonce = 0x1234; + const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; + + const receipt = await (await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce)).wait(); + let authEvent; + for (const event of receipt.logs) { + const parsedLog = zksyncContract.interface.parseLog(event); + if (parsedLog && parsedLog.name === "FactAuth") { + authEvent = parsedLog; + break; + } + } + + expect(authEvent.values.sender, "event sender incorrect").eq(wallet.address); + expect(authEvent.values.nonce, "event nonce incorrect").eq(nonce); + expect(authEvent.values.fact, "event fact incorrect").eq(pubkeyHash); + }); + + it("Auth pubkey rewrite fail", async () => { + zksyncContract.connect(wallet); + + const nonce = 0xdead; + const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; + + await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); + // + const otherPubkeyHash = "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(otherPubkeyHash, nonce)); + expect(revertReason, "revert reason incorrect").eq("ahf11"); + }); + + it("Auth pubkey incorrect length fail", async () => { + zksyncContract.connect(wallet); + const nonce = 0x7656; + const shortPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefe"; + const longPubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefefe"; + + for (const pkHash of [shortPubkeyHash, longPubkeyHash]) { + const {revertReason} = await getCallRevertReason(async () => await zksyncContract.setAuthPubkeyHash(shortPubkeyHash, nonce)); + expect(revertReason, "revert reason incorrect").eq("ahf10"); + } + }); +}); + +describe("zkSync test process next operation", function() { + this.timeout(50000); + + let zksyncContract; + let tokenContract; + let incorrectTokenContract; + let ethProxy; + before(async () => { + const contracts = readTestContracts(); + contracts.zkSync = readContractCode("ZkSyncProcessOpUnitTest"); + const deployer = new Deployer({deployWallet: wallet, contracts}); + await deployer.deployAll({gasLimit: 6500000}); + zksyncContract = deployer.zkSyncContract(wallet); + + tokenContract = await deployContract( + wallet, + readContractCode("TEST-ERC20"), [], + {gasLimit: 5000000}, + ); + await tokenContract.mint(wallet.address, parseEther("1000000")); + + const govContract = deployer.governanceContract(wallet); + await govContract.addToken(tokenContract.address); + + ethProxy = new ETHProxy(wallet.provider, { + mainContract: zksyncContract.address, + govContract: govContract.address + }); + + incorrectTokenContract = await deployContract( + wallet, + readContractCode("TEST-ERC20"), [], + {gasLimit: 5000000}, + ); + await tokenContract.mint(wallet.address, parseEther("1000000")); + }); + + it("Process noop", async () => { + zksyncContract.connect(wallet); + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + const pubdata = Buffer.alloc(CHUNK_SIZE, 0); + await zksyncContract.testProcessOperation(pubdata, "0x", []); + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); + }); + + it("Process transfer", async () => { + zksyncContract.connect(wallet); + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + const pubdata = Buffer.alloc(CHUNK_SIZE * 2, 0xff); + pubdata[0] = 0x05; + await zksyncContract.testProcessOperation(pubdata, "0x", []); + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); + }); + it("Process transfer to new", async () => { + zksyncContract.connect(wallet); + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0xff); + pubdata[0] = 0x02; + await zksyncContract.testProcessOperation(pubdata, "0x", []); + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); + }); + + it("Process deposit", async () => { + zksyncContract.connect(wallet); + const depositAmount = bigNumberify("2"); + + await zksyncContract.depositETH(wallet.address, {value: depositAmount}); + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + // construct deposit pubdata + const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); + pubdata[0] = 0x01; + let offset = 1; + pubdata.writeUInt32BE(0xccaabbff, offset); + offset += 4; + pubdata.writeUInt16BE(0, offset); // token + offset += 2; + Buffer.from(depositAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); + offset += 16; + Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); + await zksyncContract.testProcessOperation(pubdata, "0x", []); + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); + }); + + it("Process partial exit", async () => { + zksyncContract.connect(wallet); + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + // construct deposit pubdata + const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); + pubdata[0] = 0x03; + + await zksyncContract.testProcessOperation(pubdata, "0x", []); + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); + }); + + it("Process full exit", async () => { + zksyncContract.connect(wallet); + const tokenId = await ethProxy.resolveTokenId(tokenContract.address); + const fullExitAmount = parseEther("0.7"); + const accountId = 0xaabbffcc; + + await zksyncContract.fullExit(accountId, tokenContract.address); + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + // construct full exit pubdata + const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); + pubdata[0] = 0x06; + let offset = 1; + pubdata.writeUInt32BE(accountId, offset); + offset += 4; + Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); + offset += 20; + pubdata.writeUInt16BE(tokenId, offset); + offset += 2; + Buffer.from(fullExitAmount.toHexString().substr(2).padStart(16 * 2, "0"), "hex").copy(pubdata, offset); + + await zksyncContract.testProcessOperation(pubdata, "0x", []); + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter - 1, "priority request number").eq(committedPriorityRequestsBefore); + }); + + it("Change pubkey with auth", async () => { + zksyncContract.connect(wallet); + + const nonce = 0x1234; + const pubkeyHash = "0xfefefefefefefefefefefefefefefefefefefefe"; + await zksyncContract.setAuthPubkeyHash(pubkeyHash, nonce); + + const accountId = 0xffee12cc; + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + // construct deposit pubdata + const pubdata = Buffer.alloc(CHUNK_SIZE * 6, 0); + pubdata[0] = 0x07; + let offset = 1; + pubdata.writeUInt32BE(accountId, offset); + offset += 4; + Buffer.from(pubkeyHash.substr(2), "hex").copy(pubdata, offset); + offset += 20; + Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); + offset += 20; + pubdata.writeUInt32BE(nonce, offset); + + await zksyncContract.testProcessOperation(pubdata, "0x", [0]); + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); + }); + + it("Change pubkey with posted signature", async () => { + zksyncContract.connect(wallet); + + const nonce = 0x1234; + const pubkeyHash = "sync:fefefefefefefefefefefefefefefefefefefefe"; + const accountId = 0x00ffee12; + const ethWitness = await zksync.utils.signChangePubkeyMessage(wallet, pubkeyHash, nonce, accountId); + + const committedPriorityRequestsBefore = await zksyncContract.totalCommittedPriorityRequests(); + + // construct deposit pubdata + const pubdata = Buffer.alloc( CHUNK_SIZE * 6, 0); + pubdata[0] = 0x07; + let offset = 1; + pubdata.writeUInt32BE(accountId, offset); + offset += 4; + Buffer.from(pubkeyHash.substr(5), "hex").copy(pubdata, offset); + offset += 20; + Buffer.from(wallet.address.substr(2), "hex").copy(pubdata, offset); + offset += 20; + pubdata.writeUInt32BE(nonce, offset); + + await zksyncContract.testProcessOperation(pubdata, ethWitness, [(ethWitness.length - 2) / 2]); // (ethWitness.length - 2) / 2 == len of ethWitness in bytes + + const committedPriorityRequestsAfter = await zksyncContract.totalCommittedPriorityRequests(); + expect(committedPriorityRequestsAfter, "priority request number").eq(committedPriorityRequestsBefore); + }); +}); From b610d1e10e6a8b8f8930e1fc6c1717abe78cc38e Mon Sep 17 00:00:00 2001 From: Ihor Barenblat Date: Wed, 3 Jun 2020 14:01:19 +0300 Subject: [PATCH 3/5] withdrawERC20Guarded call from withdrawERC20 external function --- contracts/contracts/ZkSync.sol | 7 ++----- contracts/test/unit_tests/zksync_test.ts | 4 +++- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/contracts/contracts/ZkSync.sol b/contracts/contracts/ZkSync.sol index 30a51585f8..7439e7b372 100644 --- a/contracts/contracts/ZkSync.sol +++ b/contracts/contracts/ZkSync.sol @@ -230,11 +230,8 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { uint16 tokenId = governance.validateTokenAddress(address(_token)); bytes22 packedBalanceKey = packAddressAndTokenId(msg.sender, tokenId); uint128 balance = balancesToWithdraw[packedBalanceKey].balanceToWithdraw; - (bool sent, bytes memory withdrawnAmountEncoded) = address(this).call( - abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128)", _token, msg.sender, _amount, balance) - ); - require(sent, "wt20"); // wt20 - withdrawERC20Guarded call fails - registerSingleWithdrawal(tokenId, abi.decode(withdrawnAmountEncoded, (uint128))); + uint128 withdrawnAmount = this.withdrawERC20Guarded(_token, msg.sender, _amount, balance); + registerSingleWithdrawal(tokenId, withdrawnAmount); } /// @notice Register full exit request - pack pubdata, add priority request diff --git a/contracts/test/unit_tests/zksync_test.ts b/contracts/test/unit_tests/zksync_test.ts index 05279c57e9..e4125be78f 100644 --- a/contracts/test/unit_tests/zksync_test.ts +++ b/contracts/test/unit_tests/zksync_test.ts @@ -349,8 +349,10 @@ describe("zkSync withdraw unit tests", function() { await zksyncContract.setBalanceToWithdraw(wallet.address, tokenId, withdrawAmount); + const onchainBalBefore = await onchainBalance(wallet, tokenContract.address); const {revertReason} = await getCallRevertReason(async () => await performWithdraw(wallet, tokenContract.address, tokenId, withdrawAmount.add(1))); - expect(revertReason, "wrong revert reason").eq("wt20"); + const onchainBalAfter = await onchainBalance(wallet, tokenContract.address); + expect(onchainBalAfter).eq(onchainBalBefore); }); it("Withdraw ERC20 unsupported token", async () => { From 7944bad7e4c794b8ab6b0f54220725d243b2b1fa Mon Sep 17 00:00:00 2001 From: Ihor Barenblat Date: Wed, 3 Jun 2020 15:14:41 +0300 Subject: [PATCH 4/5] comments and spaces --- contracts/contracts/Proxy.sol | 4 ++-- contracts/contracts/UpgradeGatekeeper.sol | 8 ++++---- contracts/contracts/ZkSync.sol | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contracts/contracts/Proxy.sol b/contracts/contracts/Proxy.sol index 5fbc28d13a..81aaf98517 100644 --- a/contracts/contracts/Proxy.sol +++ b/contracts/contracts/Proxy.sol @@ -55,7 +55,7 @@ contract Proxy is Upgradeable, UpgradeableMaster, Ownable { /// @notice Upgrades target /// @param newTarget New target - /// @param newTargetUpgradeParameters New target initialization parameters + /// @param newTargetUpgradeParameters New target upgrade parameters function upgradeTarget(address newTarget, bytes calldata newTargetUpgradeParameters) external { requireMaster(msg.sender); @@ -63,7 +63,7 @@ contract Proxy is Upgradeable, UpgradeableMaster, Ownable { (bool upgradeSuccess, ) = getTarget().delegatecall( abi.encodeWithSignature("upgrade(bytes)", newTargetUpgradeParameters) ); - require(upgradeSuccess, "ufu11"); // ufu11 - target initialization failed + require(upgradeSuccess, "ufu11"); // ufu11 - target upgrade failed } /// @notice Performs a delegatecall to the contract implementation diff --git a/contracts/contracts/UpgradeGatekeeper.sol b/contracts/contracts/UpgradeGatekeeper.sol index 9496e4297c..2838679aff 100644 --- a/contracts/contracts/UpgradeGatekeeper.sol +++ b/contracts/contracts/UpgradeGatekeeper.sol @@ -100,18 +100,18 @@ contract UpgradeGatekeeper is UpgradeEvents, Ownable { } /// @notice Finishes upgrade - /// @param targetsInitializationParameters New targets initialization parameters per each upgradeable contract - function finishUpgrade(bytes[] calldata targetsInitializationParameters) external { + /// @param targetsUpgradeParameters New targets upgrade parameters per each upgradeable contract + function finishUpgrade(bytes[] calldata targetsUpgradeParameters) external { requireMaster(msg.sender); require(upgradeStatus == UpgradeStatus.Preparation, "fpu11"); // fpu11 - unable to finish upgrade without preparation status active - require(targetsInitializationParameters.length == managedContracts.length, "fpu12"); // fpu12 - number of new targets initialization parameters must be equal to the number of managed contracts + require(targetsUpgradeParameters.length == managedContracts.length, "fpu12"); // fpu12 - number of new targets upgrade parameters must be equal to the number of managed contracts require(mainContract.isReadyForUpgrade(), "fpu13"); // fpu13 - main contract is not ready for upgrade mainContract.upgradeFinishes(); for (uint64 i = 0; i < managedContracts.length; i++) { address newTarget = nextTargets[i]; if (newTarget != address(0)) { - managedContracts[i].upgradeTarget(newTarget, targetsInitializationParameters[i]); + managedContracts[i].upgradeTarget(newTarget, targetsUpgradeParameters[i]); } } versionId++; diff --git a/contracts/contracts/ZkSync.sol b/contracts/contracts/ZkSync.sol index 856dc79046..26acb94f14 100644 --- a/contracts/contracts/ZkSync.sol +++ b/contracts/contracts/ZkSync.sol @@ -723,7 +723,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { // Expiration block is: current block number + priority expiration delta uint256 expirationBlock = block.number + PRIORITY_EXPIRATION; - uint64 nextPriorityRequestId = firstPriorityRequestId + totalOpenPriorityRequests; + uint64 nextPriorityRequestId = firstPriorityRequestId + totalOpenPriorityRequests; priorityRequests[nextPriorityRequestId] = PriorityOperation({ opType: _opType, From 1af99170f4f1f05d98d305447b751d09443359ff Mon Sep 17 00:00:00 2001 From: Ihor Barenblat Date: Wed, 3 Jun 2020 15:51:52 +0300 Subject: [PATCH 5/5] comment near sendERC20 function (Utils.sol) --- contracts/contracts/Utils.sol | 9 ++++++--- contracts/contracts/ZkSync.sol | 7 +++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/contracts/contracts/Utils.sol b/contracts/contracts/Utils.sol index f1351d597e..eae485588a 100644 --- a/contracts/contracts/Utils.sol +++ b/contracts/contracts/Utils.sol @@ -1,5 +1,6 @@ pragma solidity ^0.5.0; +import "./IERC20.sol"; import "./Bytes.sol"; library Utils { @@ -14,12 +15,14 @@ library Utils { } /// @notice Sends tokens + /// @dev NOTE: this function handles tokens that have transfer function not strictly compatible with ERC20 standard + /// @dev NOTE: call `transfer` to this token may return (bool) or nothing /// @param _token Token address /// @param _to Address of recipient /// @param _amount Amount of tokens to transfer /// @return bool flag indicating that transfer is successful - function sendERC20(address _token, address _to, uint256 _amount) internal returns (bool) { - (bool callSuccess, bytes memory callReturnValueEncoded) = _token.call( + function sendERC20(IERC20 _token, address _to, uint256 _amount) internal returns (bool) { + (bool callSuccess, bytes memory callReturnValueEncoded) = address(_token).call( abi.encodeWithSignature("transfer(address,uint256)", _to, _amount) ); // `transfer` method may return (bool) or nothing. @@ -35,7 +38,7 @@ library Utils { // TODO: Use constant from Config uint256 ETH_WITHDRAWAL_GAS_LIMIT = 10000; - (bool callSuccess,) = _to.call.gas(ETH_WITHDRAWAL_GAS_LIMIT).value(_amount)(""); + (bool callSuccess, ) = _to.call.gas(ETH_WITHDRAWAL_GAS_LIMIT).value(_amount)(""); return callSuccess; } diff --git a/contracts/contracts/ZkSync.sol b/contracts/contracts/ZkSync.sol index 26acb94f14..56b976c367 100644 --- a/contracts/contracts/ZkSync.sol +++ b/contracts/contracts/ZkSync.sol @@ -1,6 +1,5 @@ pragma solidity ^0.5.0; -import "./IERC20.sol"; import "./ReentrancyGuard.sol"; import "./SafeMath.sol"; import "./SafeMathUInt128.sol"; @@ -97,7 +96,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { require(msg.sender == address(this), "wtg10"); // wtg10 - can be called only from this contract as one "external" call (to revert all this function state changes if it is needed) uint256 balance_before = _token.balanceOf(address(this)); - require(Utils.sendERC20(address(_token), _to, _amount), "wtg11"); // wtg11 - ERC20 transfer fails + require(Utils.sendERC20(_token, _to, _amount), "wtg11"); // wtg11 - ERC20 transfer fails uint256 balance_after = _token.balanceOf(address(this)); require(balance_before.sub(balance_after) <= _maxAmount, "wtg12"); // wtg12 - rollup balance difference (before and after transfer) is bigger than _maxAmount @@ -135,7 +134,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { } else { address tokenAddr = governance.tokenAddresses(tokenId); // we can just check that call not reverts because it wants to withdraw all amount - (sent,) = address(this).call.gas(ERC20_WITHDRAWAL_GAS_LIMIT)( + (sent, ) = address(this).call.gas(ERC20_WITHDRAWAL_GAS_LIMIT)( abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128)", tokenAddr, to, amount, amount) ); } @@ -177,7 +176,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { /// @param _amount Ether amount to withdraw function withdrawETH(uint128 _amount) external nonReentrant { registerWithdrawal(0, _amount, msg.sender); - (bool success,) = msg.sender.call.value(_amount)(""); + (bool success, ) = msg.sender.call.value(_amount)(""); require(success, "fwe11"); // ETH withdraw failed }