Merge pull request #643 from matter-labs/ib/638/ERC20_withdraw_guard

ERC20 withdraw guard

contracts/contracts/Proxy.sol

@@ -55,15 +55,15 @@ 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);
 
         setTarget(newTarget);
         (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

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++;

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;
     }
 

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,

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);
+    }
+
+}

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);
+    }
+
+}

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;
+    }
+
+}

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);