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/Utils.sol b/contracts/contracts/Utils.sol index ae23ab6e65..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,15 +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 sendERC20NoRevert(address _token, address _to, uint256 _amount) internal returns (bool) { - // TODO: Use constant from Config - uint256 ERC20_WITHDRAWAL_GAS_LIMIT = 250000; - - (bool callSuccess, bytes memory callReturnValueEncoded) = _token.call.gas(ERC20_WITHDRAWAL_GAS_LIMIT)( + 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. @@ -38,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 018c0711cd..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"; @@ -87,6 +86,23 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { revert("upgzk"); // it is the first version, upgrade is not supported, use initialize } + /// @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(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 + + return SafeCast.toUint128(balance_before.sub(balance_after)); + } + /// @notice executes pending withdrawals /// @param _n The number of withdrawals to complete starting from oldest function completeWithdrawals(uint32 _n) external nonReentrant { @@ -117,8 +133,10 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { sent = Utils.sendETHNoRevert(toPayable, amount); } else { address tokenAddr = governance.tokenAddresses(tokenId); - require(tokenAddr != address(0), "cwd11"); // unknown tokenId - sent = Utils.sendERC20NoRevert(tokenAddr, to, amount); + // we can just check that call not reverts because it wants to withdraw all amount + (sent, ) = address(this).call.gas(ERC20_WITHDRAWAL_GAS_LIMIT)( + abi.encodeWithSignature("withdrawERC20Guarded(address,address,uint128,uint128)", tokenAddr, to, amount, amount) + ); } if (!sent) { balancesToWithdraw[packedBalanceKey].balanceToWithdraw += amount; @@ -158,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 } @@ -185,8 +203,10 @@ 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)); - registerWithdrawal(tokenId, _amount, msg.sender); - require(_token.transfer(msg.sender, _amount), "fw011"); // token transfer failed withdraw + bytes22 packedBalanceKey = packAddressAndTokenId(msg.sender, tokenId); + uint128 balance = balancesToWithdraw[packedBalanceKey].balanceToWithdraw; + uint128 withdrawnAmount = this.withdrawERC20Guarded(_token, msg.sender, _amount, balance); + registerWithdrawal(tokenId, withdrawnAmount, msg.sender); } /// @notice Register full exit request - pack pubdata, add priority request @@ -432,11 +452,11 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { ); } - function emitDepositCommit(uint32 _blockNumber, Operations.Deposit memory depositData) internal { + function emitDepositCommitEvent(uint32 _blockNumber, Operations.Deposit memory depositData) internal { emit DepositCommit(_blockNumber, depositData.accountId, depositData.owner, depositData.tokenId, depositData.amount); } - function emitFullExitCommit(uint32 _blockNumber, Operations.FullExit memory fullExitData) internal { + function emitFullExitCommitEvent(uint32 _blockNumber, Operations.FullExit memory fullExitData) internal { emit FullExitCommit(_blockNumber, fullExitData.accountId, fullExitData.owner, fullExitData.tokenId, fullExitData.amount); } @@ -489,7 +509,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { bytes memory pubData = Bytes.slice(_publicData, pubdataOffset + 1, DEPOSIT_BYTES - 1); Operations.Deposit memory depositData = Operations.readDepositPubdata(pubData); - emitDepositCommit(_blockNumber, depositData); + emitDepositCommitEvent(_blockNumber, depositData); OnchainOperation memory onchainOp = OnchainOperation( Operations.OpType.Deposit, @@ -510,7 +530,7 @@ contract ZkSync is UpgradeableMaster, Storage, Config, Events, ReentrancyGuard { bytes memory pubData = Bytes.slice(_publicData, pubdataOffset + 1, FULL_EXIT_BYTES - 1); Operations.FullExit memory fullExitData = Operations.readFullExitPubdata(pubData); - emitFullExitCommit(_blockNumber, fullExitData); + emitFullExitCommitEvent(_blockNumber, fullExitData); bool addToPendingWithdrawalsQueue = false; withdrawalsDataHash = keccak256(abi.encode(withdrawalsDataHash, addToPendingWithdrawalsQueue, fullExitData.owner, fullExitData.tokenId, fullExitData.amount)); @@ -702,7 +722,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, diff --git a/contracts/contracts/test/ZKSyncUnitTest.sol b/contracts/contracts/test/ZKSyncUnitTest.sol deleted file mode 100644 index 07718882cc..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 Utils.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..f1502da1b1 --- /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 Utils.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..e4125be78f 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); @@ -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("SafeMath: subtraction overflow"); + const onchainBalAfter = await onchainBalance(wallet, tokenContract.address); + expect(onchainBalAfter).eq(onchainBalBefore); }); it("Withdraw ERC20 unsupported token", async () => { @@ -399,7 +401,6 @@ describe("zkSync auth pubkey onchain unit tests", function() { 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); @@ -476,7 +477,7 @@ describe("zkSync test process next operation", function() { let ethProxy; before(async () => { const contracts = readTestContracts(); - contracts.zkSync = readContractCode("ZKSyncUnitTest"); + contracts.zkSync = readContractCode("ZkSyncProcessOpUnitTest"); const deployer = new Deployer({deployWallet: wallet, contracts}); await deployer.deployAll({gasLimit: 6500000}); zksyncContract = deployer.zkSyncContract(wallet);