diff --git a/contracts/legacy/LegacyTransferManager.sol b/contracts/legacy/LegacyTransferManager.sol new file mode 100644 index 000000000..e925de49e --- /dev/null +++ b/contracts/legacy/LegacyTransferManager.sol @@ -0,0 +1,501 @@ +// Copyright (C) 2018 Argent Labs Ltd. + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +pragma solidity ^0.5.4; +import "../wallet/BaseWallet.sol"; +import "../modules/common/BaseModule.sol"; +import "../modules/common/RelayerModule.sol"; +import "../modules/common/OnlyOwnerModule.sol"; +import "../modules/common/BaseTransfer.sol"; +import "../modules/common/LimitManager.sol"; +import "../infrastructure/TokenPriceProvider.sol"; +import "../modules/storage/TransferStorage.sol"; +import "../../lib/other/ERC20.sol"; + +/** + * @title LegacyTransferManager + * @dev Copy of TransferManager module as from release 1.5 + */ +contract LegacyTransferManager is BaseModule, RelayerModule, OnlyOwnerModule, BaseTransfer, LimitManager { + + bytes32 constant NAME = "TransferManager"; + + bytes4 private constant ERC1271_ISVALIDSIGNATURE_BYTES = bytes4(keccak256("isValidSignature(bytes,bytes)")); + bytes4 private constant ERC1271_ISVALIDSIGNATURE_BYTES32 = bytes4(keccak256("isValidSignature(bytes32,bytes)")); + + enum ActionType { Transfer } + + using SafeMath for uint256; + + struct TokenManagerConfig { + // Mapping between pending action hash and their timestamp + mapping (bytes32 => uint256) pendingActions; + } + + // wallet specific storage + mapping (address => TokenManagerConfig) internal configs; + + // The security period + uint256 public securityPeriod; + // The execution window + uint256 public securityWindow; + // The Token storage + TransferStorage public transferStorage; + // The Token price provider + TokenPriceProvider public priceProvider; + // The previous limit manager needed to migrate the limits + LimitManager public oldLimitManager; + + // *************** Events *************************** // + + event AddedToWhitelist(address indexed wallet, address indexed target, uint64 whitelistAfter); + event RemovedFromWhitelist(address indexed wallet, address indexed target); + event PendingTransferCreated(address indexed wallet, bytes32 indexed id, uint256 indexed executeAfter, + address token, address to, uint256 amount, bytes data); + event PendingTransferExecuted(address indexed wallet, bytes32 indexed id); + event PendingTransferCanceled(address indexed wallet, bytes32 indexed id); + + // *************** Constructor ********************** // + + constructor( + ModuleRegistry _registry, + TransferStorage _transferStorage, + GuardianStorage _guardianStorage, + address _priceProvider, + uint256 _securityPeriod, + uint256 _securityWindow, + uint256 _defaultLimit, + LimitManager _oldLimitManager + ) + BaseModule(_registry, _guardianStorage, NAME) + LimitManager(_defaultLimit) + public + { + transferStorage = _transferStorage; + priceProvider = TokenPriceProvider(_priceProvider); + securityPeriod = _securityPeriod; + securityWindow = _securityWindow; + oldLimitManager = _oldLimitManager; + } + + /** + * @dev Inits the module for a wallet by setting up the isValidSignature (EIP 1271) + * static call redirection from the wallet to the module and copying all the parameters + * of the daily limit from the previous implementation of the LimitManager module. + * @param _wallet The target wallet. + */ + function init(BaseWallet _wallet) public onlyWallet(_wallet) { + + // setup static calls + _wallet.enableStaticCall(address(this), ERC1271_ISVALIDSIGNATURE_BYTES); + _wallet.enableStaticCall(address(this), ERC1271_ISVALIDSIGNATURE_BYTES32); + + // setup default limit for new deployment + if (address(oldLimitManager) == address(0)) { + super.init(_wallet); + return; + } + // get limit from previous LimitManager + uint256 current = oldLimitManager.getCurrentLimit(_wallet); + (uint256 pending, uint64 changeAfter) = oldLimitManager.getPendingLimit(_wallet); + // setup default limit for new wallets + if (current == 0 && changeAfter == 0) { + super.init(_wallet); + return; + } + // migrate existing limit for existing wallets + if (current == pending) { + limits[address(_wallet)].limit.current = uint128(current); + } else { + limits[address(_wallet)].limit = Limit(uint128(current), uint128(pending), changeAfter); + } + // migrate daily pending if we are within a rolling period + (uint256 unspent, uint64 periodEnd) = oldLimitManager.getDailyUnspent(_wallet); + // solium-disable-next-line security/no-block-members + if (periodEnd > now) { + limits[address(_wallet)].dailySpent = DailySpent(uint128(current.sub(unspent)), periodEnd); + } + } + + // *************** External/Public Functions ********************* // + + /** + * @dev lets the owner transfer tokens (ETH or ERC20) from a wallet. + * @param _wallet The target wallet. + * @param _token The address of the token to transfer. + * @param _to The destination address + * @param _amount The amoutn of token to transfer + * @param _data The data for the transaction + */ + function transferToken( + BaseWallet _wallet, + address _token, + address _to, + uint256 _amount, + bytes calldata _data + ) + external + onlyWalletOwner(_wallet) + onlyWhenUnlocked(_wallet) + { + if (isWhitelisted(_wallet, _to)) { + // transfer to whitelist + doTransfer(_wallet, _token, _to, _amount, _data); + } else { + uint256 etherAmount = (_token == ETH_TOKEN) ? _amount : priceProvider.getEtherValue(_amount, _token); + if (checkAndUpdateDailySpent(_wallet, etherAmount)) { + // transfer under the limit + doTransfer(_wallet, _token, _to, _amount, _data); + } else { + // transfer above the limit + (bytes32 id, uint256 executeAfter) = addPendingAction(ActionType.Transfer, _wallet, _token, _to, _amount, _data); + emit PendingTransferCreated(address(_wallet), id, executeAfter, _token, _to, _amount, _data); + } + } + } + + /** + * @dev lets the owner approve an allowance of ERC20 tokens for a spender (dApp). + * @param _wallet The target wallet. + * @param _token The address of the token to transfer. + * @param _spender The address of the spender + * @param _amount The amount of tokens to approve + */ + function approveToken( + BaseWallet _wallet, + address _token, + address _spender, + uint256 _amount + ) + external + onlyWalletOwner(_wallet) + onlyWhenUnlocked(_wallet) + { + if (isWhitelisted(_wallet, _spender)) { + // approve to whitelist + doApproveToken(_wallet, _token, _spender, _amount); + } else { + // get current alowance + uint256 currentAllowance = ERC20(_token).allowance(address(_wallet), _spender); + if (_amount <= currentAllowance) { + // approve if we reduce the allowance + doApproveToken(_wallet, _token, _spender, _amount); + } else { + // check if delta is under the limit + uint delta = _amount - currentAllowance; + uint256 deltaInEth = priceProvider.getEtherValue(delta, _token); + require(checkAndUpdateDailySpent(_wallet, deltaInEth), "TM: Approve above daily limit"); + // approve if under the limit + doApproveToken(_wallet, _token, _spender, _amount); + } + } + } + + /** + * @dev lets the owner call a contract. + * @param _wallet The target wallet. + * @param _contract The address of the contract. + * @param _value The amount of ETH to transfer as part of call + * @param _data The encoded method data + */ + function callContract( + BaseWallet _wallet, + address _contract, + uint256 _value, + bytes calldata _data + ) + external + onlyWalletOwner(_wallet) + onlyWhenUnlocked(_wallet) + { + // Make sure we don't call a module, the wallet itself, or a supported ERC20 + authoriseContractCall(_wallet, _contract); + + if (isWhitelisted(_wallet, _contract)) { + // call to whitelist + doCallContract(_wallet, _contract, _value, _data); + } else { + require(checkAndUpdateDailySpent(_wallet, _value), "TM: Call contract above daily limit"); + // call under the limit + doCallContract(_wallet, _contract, _value, _data); + } + } + + /** + * @dev lets the owner do an ERC20 approve followed by a call to a contract. + * We assume that the contract will pull the tokens and does not require ETH. + * @param _wallet The target wallet. + * @param _token The token to approve. + * @param _contract The address of the contract. + * @param _amount The amount of ERC20 tokens to approve. + * @param _data The encoded method data + */ + function approveTokenAndCallContract( + BaseWallet _wallet, + address _token, + address _contract, + uint256 _amount, + bytes calldata _data + ) + external + onlyWalletOwner(_wallet) + onlyWhenUnlocked(_wallet) + { + // Make sure we don't call a module, the wallet itself, or a supported ERC20 + authoriseContractCall(_wallet, _contract); + + if (isWhitelisted(_wallet, _contract)) { + doApproveToken(_wallet, _token, _contract, _amount); + doCallContract(_wallet, _contract, 0, _data); + } else { + // get current alowance + uint256 currentAllowance = ERC20(_token).allowance(address(_wallet), _contract); + if (_amount <= currentAllowance) { + // no need to approve more + doCallContract(_wallet, _contract, 0, _data); + } else { + // check if delta is under the limit + uint delta = _amount - currentAllowance; + uint256 deltaInEth = priceProvider.getEtherValue(delta, _token); + require(checkAndUpdateDailySpent(_wallet, deltaInEth), "TM: Approve above daily limit"); + // approve if under the limit + doApproveToken(_wallet, _token, _contract, _amount); + doCallContract(_wallet, _contract, 0, _data); + } + } + } + + /** + * @dev Adds an address to the whitelist of a wallet. + * @param _wallet The target wallet. + * @param _target The address to add. + */ + function addToWhitelist( + BaseWallet _wallet, + address _target + ) + external + onlyWalletOwner(_wallet) + onlyWhenUnlocked(_wallet) + { + require(!isWhitelisted(_wallet, _target), "TT: target already whitelisted"); + // solium-disable-next-line security/no-block-members + uint256 whitelistAfter = now.add(securityPeriod); + transferStorage.setWhitelist(_wallet, _target, whitelistAfter); + emit AddedToWhitelist(address(_wallet), _target, uint64(whitelistAfter)); + } + + /** + * @dev Removes an address from the whitelist of a wallet. + * @param _wallet The target wallet. + * @param _target The address to remove. + */ + function removeFromWhitelist( + BaseWallet _wallet, + address _target + ) + external + onlyWalletOwner(_wallet) + onlyWhenUnlocked(_wallet) + { + require(isWhitelisted(_wallet, _target), "TT: target not whitelisted"); + transferStorage.setWhitelist(_wallet, _target, 0); + emit RemovedFromWhitelist(address(_wallet), _target); + } + + /** + * @dev Executes a pending transfer for a wallet. + * The method can be called by anyone to enable orchestration. + * @param _wallet The target wallet. + * @param _token The token of the pending transfer. + * @param _to The destination address of the pending transfer. + * @param _amount The amount of token to transfer of the pending transfer. + * @param _data The data associated to the pending transfer. + * @param _block The block at which the pending transfer was created. + */ + function executePendingTransfer( + BaseWallet _wallet, + address _token, + address _to, + uint _amount, + bytes calldata _data, + uint _block + ) + external + onlyWhenUnlocked(_wallet) + { + bytes32 id = keccak256(abi.encodePacked(ActionType.Transfer, _token, _to, _amount, _data, _block)); + uint executeAfter = configs[address(_wallet)].pendingActions[id]; + require(executeAfter > 0, "TT: unknown pending transfer"); + uint executeBefore = executeAfter.add(securityWindow); + // solium-disable-next-line security/no-block-members + require(executeAfter <= now && now <= executeBefore, "TT: transfer outside of the execution window"); + delete configs[address(_wallet)].pendingActions[id]; + doTransfer(_wallet, _token, _to, _amount, _data); + emit PendingTransferExecuted(address(_wallet), id); + } + + function cancelPendingTransfer( + BaseWallet _wallet, + bytes32 _id + ) + external + onlyWalletOwner(_wallet) + onlyWhenUnlocked(_wallet) + { + require(configs[address(_wallet)].pendingActions[_id] > 0, "TT: unknown pending action"); + delete configs[address(_wallet)].pendingActions[_id]; + emit PendingTransferCanceled(address(_wallet), _id); + } + + /** + * @dev Lets the owner of a wallet change its daily limit. + * The limit is expressed in ETH. Changes to the limit take 24 hours. + * @param _wallet The target wallet. + * @param _newLimit The new limit. + */ + function changeLimit(BaseWallet _wallet, uint256 _newLimit) external onlyWalletOwner(_wallet) onlyWhenUnlocked(_wallet) { + changeLimit(_wallet, _newLimit, securityPeriod); + } + + /** + * @dev Convenience method to disable the limit + * The limit is disabled by setting it to an arbitrary large value. + * @param _wallet The target wallet. + */ + function disableLimit(BaseWallet _wallet) external onlyWalletOwner(_wallet) onlyWhenUnlocked(_wallet) { + disableLimit(_wallet, securityPeriod); + } + + /** + * @dev Checks if an address is whitelisted for a wallet. + * @param _wallet The target wallet. + * @param _target The address. + * @return true if the address is whitelisted. + */ + function isWhitelisted(BaseWallet _wallet, address _target) public view returns (bool _isWhitelisted) { + uint whitelistAfter = transferStorage.getWhitelist(_wallet, _target); + // solium-disable-next-line security/no-block-members + return whitelistAfter > 0 && whitelistAfter < now; + } + + /** + * @dev Gets the info of a pending transfer for a wallet. + * @param _wallet The target wallet. + * @param _id The pending transfer ID. + * @return the epoch time at which the pending transfer can be executed. + */ + function getPendingTransfer(BaseWallet _wallet, bytes32 _id) external view returns (uint64 _executeAfter) { + _executeAfter = uint64(configs[address(_wallet)].pendingActions[_id]); + } + + /** + * @dev Implementation of EIP 1271. + * Should return whether the signature provided is valid for the provided data. + * @param _data Arbitrary length data signed on the behalf of address(this) + * @param _signature Signature byte array associated with _data + */ + function isValidSignature(bytes calldata _data, bytes calldata _signature) external view returns (bytes4) { + bytes32 msgHash = keccak256(abi.encodePacked(_data)); + isValidSignature(msgHash, _signature); + return ERC1271_ISVALIDSIGNATURE_BYTES; + } + + /** + * @dev Implementation of EIP 1271. + * Should return whether the signature provided is valid for the provided data. + * @param _msgHash Hash of a message signed on the behalf of address(this) + * @param _signature Signature byte array associated with _msgHash + */ + function isValidSignature(bytes32 _msgHash, bytes memory _signature) public view returns (bytes4) { + require(_signature.length == 65, "TM: invalid signature length"); + address signer = recoverSigner(_msgHash, _signature, 0); + require(isOwner(BaseWallet(msg.sender), signer), "TM: Invalid signer"); + return ERC1271_ISVALIDSIGNATURE_BYTES32; + } + + // *************** Internal Functions ********************* // + + /** + * @dev Creates a new pending action for a wallet. + * @param _action The target action. + * @param _wallet The target wallet. + * @param _token The target token for the action. + * @param _to The recipient of the action. + * @param _amount The amount of token associated to the action. + * @param _data The data associated to the action. + * @return the identifier for the new pending action and the time when the action can be executed + */ + function addPendingAction( + ActionType _action, + BaseWallet _wallet, + address _token, + address _to, + uint _amount, + bytes memory _data + ) + internal + returns (bytes32 id, uint256 executeAfter) + { + id = keccak256(abi.encodePacked(_action, _token, _to, _amount, _data, block.number)); + require(configs[address(_wallet)].pendingActions[id] == 0, "TM: duplicate pending action"); + // solium-disable-next-line security/no-block-members + executeAfter = now.add(securityPeriod); + configs[address(_wallet)].pendingActions[id] = executeAfter; + } + + /** + * @dev Make sure a contract call is not trying to call a module, the wallet itself, or a supported ERC20. + * @param _wallet The target wallet. + * @param _contract The address of the contract. + */ + function authoriseContractCall(BaseWallet _wallet, address _contract) internal view { + require( + _contract != address(_wallet) && // not the wallet itself + !_wallet.authorised(_contract) && // not an authorised module + (priceProvider.cachedPrices(_contract) == 0 || isLimitDisabled(_wallet)), // not an ERC20 listed in the provider (or limit disabled) + "TM: Forbidden contract"); + } + + // *************** Implementation of RelayerModule methods ********************* // + + // Overrides refund to add the refund in the daily limit. + function refund(BaseWallet _wallet, uint _gasUsed, uint _gasPrice, uint _gasLimit, uint _signatures, address _relayer) internal { + // 21000 (transaction) + 7620 (execution of refund) + 7324 (execution of updateDailySpent) + 672 to log the event + _gasUsed + uint256 amount = 36616 + _gasUsed; + if (_gasPrice > 0 && _signatures > 0 && amount <= _gasLimit) { + if (_gasPrice > tx.gasprice) { + amount = amount * tx.gasprice; + } else { + amount = amount * _gasPrice; + } + checkAndUpdateDailySpent(_wallet, amount); + invokeWallet(address(_wallet), _relayer, amount, EMPTY_BYTES); + } + } + + // Overrides verifyRefund to add the refund in the daily limit. + function verifyRefund(BaseWallet _wallet, uint _gasUsed, uint _gasPrice, uint _signatures) internal view returns (bool) { + if (_gasPrice > 0 && _signatures > 0 && ( + address(_wallet).balance < _gasUsed * _gasPrice || + isWithinDailyLimit(_wallet, getCurrentLimit(_wallet), _gasUsed * _gasPrice) == false || + _wallet.authorised(address(this)) == false + )) + { + return false; + } + return true; + } +} diff --git a/contracts/modules/ApprovedTransfer.sol b/contracts/modules/ApprovedTransfer.sol index f677b5b1d..cbfa520e0 100644 --- a/contracts/modules/ApprovedTransfer.sol +++ b/contracts/modules/ApprovedTransfer.sol @@ -18,7 +18,6 @@ import "../wallet/BaseWallet.sol"; import "./common/BaseModule.sol"; import "./common/RelayerModuleV2.sol"; import "./common/BaseTransfer.sol"; -import "../../lib/utils/SafeMath.sol"; /** * @title ApprovedTransfer @@ -100,8 +99,7 @@ contract ApprovedTransfer is BaseModule, RelayerModuleV2, BaseTransfer { onlyWhenUnlocked(_wallet) { require(!_wallet.authorised(_contract) && _contract != address(_wallet), "AT: Forbidden contract"); - doApproveToken(_wallet, _token, _spender, _amount); - doCallContract(_wallet, _contract, 0, _data); + doApproveTokenAndCallContract(_wallet, _token, _spender, _amount, _contract, _data); } // *************** Implementation of RelayerModule methods ********************* // diff --git a/contracts/modules/CompoundManager.sol b/contracts/modules/CompoundManager.sol index a874b8b71..0762803e6 100644 --- a/contracts/modules/CompoundManager.sol +++ b/contracts/modules/CompoundManager.sol @@ -15,7 +15,6 @@ pragma solidity ^0.5.4; -import "../../lib/utils/SafeMath.sol"; import "../wallet/BaseWallet.sol"; import "./common/BaseModule.sol"; import "./common/RelayerModule.sol"; diff --git a/contracts/modules/RecoveryManager.sol b/contracts/modules/RecoveryManager.sol index fd8d67d22..1a23ed98a 100644 --- a/contracts/modules/RecoveryManager.sol +++ b/contracts/modules/RecoveryManager.sol @@ -18,7 +18,6 @@ import "../wallet/BaseWallet.sol"; import "./common/BaseModule.sol"; import "./common/RelayerModuleV2.sol"; import "./storage/GuardianStorage.sol"; -import "../../lib/utils/SafeMath.sol"; /** * @title RecoveryManager diff --git a/contracts/modules/TokenExchanger.sol b/contracts/modules/TokenExchanger.sol index 27ab0938f..68c31bd1a 100644 --- a/contracts/modules/TokenExchanger.sol +++ b/contracts/modules/TokenExchanger.sol @@ -18,7 +18,6 @@ import "../wallet/BaseWallet.sol"; import "./common/BaseModule.sol"; import "./common/RelayerModule.sol"; import "./common/OnlyOwnerModule.sol"; -import "../../lib/utils/SafeMath.sol"; import "../../lib/other/ERC20.sol"; import "../../lib/other/KyberNetwork.sol"; diff --git a/contracts/modules/TransferManager.sol b/contracts/modules/TransferManager.sol index 83ed76c60..48aa3fcf3 100644 --- a/contracts/modules/TransferManager.sol +++ b/contracts/modules/TransferManager.sol @@ -240,15 +240,17 @@ contract TransferManager is BaseModule, RelayerModule, OnlyOwnerModule, BaseTran * We assume that the contract will pull the tokens and does not require ETH. * @param _wallet The target wallet. * @param _token The token to approve. - * @param _contract The address of the contract. + * @param _spender The address to approve. * @param _amount The amount of ERC20 tokens to approve. + * @param _contract The address of the contract. * @param _data The encoded method data */ function approveTokenAndCallContract( BaseWallet _wallet, address _token, - address _contract, + address _spender, uint256 _amount, + address _contract, bytes calldata _data ) external @@ -258,25 +260,14 @@ contract TransferManager is BaseModule, RelayerModule, OnlyOwnerModule, BaseTran // Make sure we don't call a module, the wallet itself, or a supported ERC20 authoriseContractCall(_wallet, _contract); - if (isWhitelisted(_wallet, _contract)) { - doApproveToken(_wallet, _token, _contract, _amount); - doCallContract(_wallet, _contract, 0, _data); - } else { - // get current alowance - uint256 currentAllowance = ERC20(_token).allowance(address(_wallet), _contract); - if (_amount <= currentAllowance) { - // no need to approve more - doCallContract(_wallet, _contract, 0, _data); - } else { - // check if delta is under the limit - uint delta = _amount - currentAllowance; - uint256 deltaInEth = priceProvider.getEtherValue(delta, _token); - require(checkAndUpdateDailySpent(_wallet, deltaInEth), "TM: Approve above daily limit"); - // approve if under the limit - doApproveToken(_wallet, _token, _contract, _amount); - doCallContract(_wallet, _contract, 0, _data); - } + if (!isWhitelisted(_wallet, _contract)) { + // check if the amount is under the daily limit + // check the entire amount because the currently approved amount will be restored and should still count towards the daily limit + uint256 valueInEth = priceProvider.getEtherValue(_amount, _token); + require(checkAndUpdateDailySpent(_wallet, valueInEth), "TM: Approve above daily limit"); } + + doApproveTokenAndCallContract(_wallet, _token, _spender, _amount, _contract, _data); } /** diff --git a/contracts/modules/common/BaseModule.sol b/contracts/modules/common/BaseModule.sol index f16a5710d..02012bb43 100644 --- a/contracts/modules/common/BaseModule.sol +++ b/contracts/modules/common/BaseModule.sol @@ -19,6 +19,7 @@ import "../../infrastructure/ModuleRegistry.sol"; import "../storage/GuardianStorage.sol"; import "./Module.sol"; import "../../../lib/other/ERC20.sol"; +import "../../../lib/utils/SafeMath.sol"; /** * @title BaseModule diff --git a/contracts/modules/common/BaseTransfer.sol b/contracts/modules/common/BaseTransfer.sol index 355a2a50c..3ed726003 100644 --- a/contracts/modules/common/BaseTransfer.sol +++ b/contracts/modules/common/BaseTransfer.sol @@ -32,7 +32,15 @@ contract BaseTransfer is BaseModule { event Transfer(address indexed wallet, address indexed token, uint256 indexed amount, address to, bytes data); event Approved(address indexed wallet, address indexed token, uint256 amount, address spender); event CalledContract(address indexed wallet, address indexed to, uint256 amount, bytes data); - + event ApprovedAndCalledContract( + address indexed wallet, + address indexed to, + address spender, + address indexed token, + uint256 amountApproved, + uint256 amountSpent, + bytes data + ); // *************** Internal Functions ********************* // /** @@ -77,4 +85,55 @@ contract BaseTransfer is BaseModule { invokeWallet(address(_wallet), _contract, _value, _data); emit CalledContract(address(_wallet), _contract, _value, _data); } + + /** + * @dev Helper method to approve a certain amount of token and call an external contract. + * The address that spends the _token and the address that is called with _data can be different. + * @param _wallet The target wallet. + * @param _token The ERC20 address. + * @param _spender The spender address. + * @param _amount The amount of tokens to transfer. + * @param _contract The contract address. + * @param _data The method data. + */ + function doApproveTokenAndCallContract( + BaseWallet _wallet, + address _token, + address _spender, + uint256 _amount, + address _contract, + bytes memory _data + ) + internal + { + uint256 existingAllowance = ERC20(_token).allowance(address(_wallet), _spender); + uint256 totalAllowance = SafeMath.add(existingAllowance, _amount); + // Approve the desired amount plus existing amount. This logic allows for potential gas saving later + // when restoring the original approved amount, in cases where the _spender uses the exact approved _amount. + bytes memory methodData = abi.encodeWithSignature("approve(address,uint256)", _spender, totalAllowance); + + invokeWallet(address(_wallet), _token, 0, methodData); + invokeWallet(address(_wallet), _contract, 0, _data); + + // Calculate the approved amount that was spent after the call + uint256 unusedAllowance = ERC20(_token).allowance(address(_wallet), _spender); + uint256 usedAllowance = SafeMath.sub(totalAllowance, unusedAllowance); + // Ensure the amount spent does not exceed the amount approved for this call + require(usedAllowance <= _amount, "BT: insufficient amount for call"); + + if (unusedAllowance != existingAllowance) { + // Restore the original allowance amount if the amount spent was different (can be lower). + methodData = abi.encodeWithSignature("approve(address,uint256)", _spender, existingAllowance); + invokeWallet(address(_wallet), _token, 0, methodData); + } + + emit ApprovedAndCalledContract( + address(_wallet), + _contract, + _spender, + _token, + _amount, + usedAllowance, + _data); + } } diff --git a/contracts/modules/common/LimitManager.sol b/contracts/modules/common/LimitManager.sol index c130a2a27..4e494b8d1 100644 --- a/contracts/modules/common/LimitManager.sol +++ b/contracts/modules/common/LimitManager.sol @@ -15,7 +15,6 @@ pragma solidity ^0.5.4; import "../../wallet/BaseWallet.sol"; -import "../../../lib/utils/SafeMath.sol"; import "./BaseModule.sol"; /** diff --git a/contracts/modules/maker/MakerManager.sol b/contracts/modules/maker/MakerManager.sol index 862fa2fec..167af8dfb 100644 --- a/contracts/modules/maker/MakerManager.sol +++ b/contracts/modules/maker/MakerManager.sol @@ -15,7 +15,6 @@ pragma solidity ^0.5.4; -import "../../../lib/utils/SafeMath.sol"; import "../../wallet/BaseWallet.sol"; import "../common/BaseModule.sol"; import "../common/RelayerModule.sol"; diff --git a/contracts/test/argent/legacy/TokenTransfer.sol b/contracts/test/argent/legacy/TokenTransfer.sol deleted file mode 100644 index 5ff453312..000000000 --- a/contracts/test/argent/legacy/TokenTransfer.sol +++ /dev/null @@ -1,375 +0,0 @@ -pragma solidity ^0.5.4; -import "../../../wallet/BaseWallet.sol"; -import "../../../modules/common/BaseModule.sol"; -import "../../../modules/common/RelayerModule.sol"; -import "../../../modules/common/LimitManager.sol"; -import "../../../infrastructure/TokenPriceProvider.sol"; -import "../../../modules/storage/GuardianStorage.sol"; -import "../../../modules/storage/TransferStorage.sol"; - -/** - * @title LegacyTokenTransfer - * @dev Legacy Module to transfer tokens (ETH or ERC20) based on a security context (daily limit, whitelist, etc). - * @author Julien Niset - - */ -contract LegacyTokenTransfer is BaseModule, RelayerModule, LimitManager { - - bytes32 constant NAME = "TokenTransfer"; - - bytes4 constant internal EXECUTE_PENDING_PREFIX = bytes4(keccak256("executePendingTransfer(address,address,address,uint256,bytes,uint256)")); - - bytes constant internal EMPTY_BYTES = ""; - - using SafeMath for uint256; - - // Mock token address for ETH - address constant internal ETH_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; - // large limit when the limit can be considered disabled - uint128 constant internal LIMIT_DISABLED = uint128(-1); // 3.40282366920938463463374607431768211455e+38 - - struct TokenTransferConfig { - // Mapping between pending transfer hash and their timestamp - mapping (bytes32 => uint256) pendingTransfers; - } - - // wallet specific storage - mapping (address => TokenTransferConfig) internal configs; - - // The security period - uint256 public securityPeriod; - // The execution window - uint256 public securityWindow; - // The Token storage - TransferStorage public transferStorage; - // The Token price provider - TokenPriceProvider public priceProvider; - - // *************** Events *************************** // - - event Transfer(address indexed wallet, address indexed token, uint256 indexed amount, address to, bytes data); - event AddedToWhitelist(address indexed wallet, address indexed target, uint64 whitelistAfter); - event RemovedFromWhitelist(address indexed wallet, address indexed target); - event PendingTransferCreated(address indexed wallet, bytes32 indexed id, uint256 indexed executeAfter, address token, address to, uint256 amount, bytes data); - event PendingTransferExecuted(address indexed wallet, bytes32 indexed id); - event PendingTransferCanceled(address indexed wallet, bytes32 indexed id); - - // *************** Constructor ********************** // - - constructor( - ModuleRegistry _registry, - TransferStorage _transferStorage, - GuardianStorage _guardianStorage, - address _priceProvider, - uint256 _securityPeriod, - uint256 _securityWindow, - uint256 _defaultLimit - ) - BaseModule(_registry, _guardianStorage, NAME) - LimitManager(_defaultLimit) - public - { - transferStorage = _transferStorage; - priceProvider = TokenPriceProvider(_priceProvider); - securityPeriod = _securityPeriod; - securityWindow = _securityWindow; - } - - // *************** External/Public Functions ********************* // - - /** - * @dev lets the owner transfer tokens (ETH or ERC20) from a wallet. - * @param _wallet The target wallet. - * @param _token The address of the token to transfer. - * @param _to The destination address - * @param _amount The amoutn of token to transfer - * @param _data The data for the transaction - */ - function transferToken( - BaseWallet _wallet, - address _token, - address _to, - uint256 _amount, - bytes calldata _data - ) - external - onlyWalletOwner(_wallet) - onlyWhenUnlocked(_wallet) - { - if(isWhitelisted(_wallet, _to)) { - // eth transfer to whitelist - if(_token == ETH_TOKEN) { - transferETH(_wallet, _to, _amount, _data); - } - // erc20 transfer to whitelist - else { - transferERC20(_wallet, _token, _to, _amount, _data); - } - } - else { - if(_token == ETH_TOKEN) { - // eth transfer under the limit - if (checkAndUpdateDailySpent(_wallet, _amount)) { - transferETH(_wallet, _to, _amount, _data); - } - // eth transfer above the limit - else { - addPendingTransfer(_wallet, ETH_TOKEN, _to, _amount, _data); - } - } - else { - uint256 etherAmount = priceProvider.getEtherValue(_amount, _token); - // erc20 transfer under the limit - if (checkAndUpdateDailySpent(_wallet, etherAmount)) { - transferERC20(_wallet, _token, _to, _amount, _data); - } - // erc20 transfer above the limit - else { - addPendingTransfer(_wallet, _token, _to, _amount, _data); - } - } - } - } - - /** - * @dev Adds an address to the whitelist of a wallet. - * @param _wallet The target wallet. - * @param _target The address to add. - */ - function addToWhitelist( - BaseWallet _wallet, - address _target - ) - external - onlyWalletOwner(_wallet) - onlyWhenUnlocked(_wallet) - { - require(!isWhitelisted(_wallet, _target), "TT: target already whitelisted"); - // solium-disable-next-line security/no-block-members - uint256 whitelistAfter = now.add(securityPeriod); - transferStorage.setWhitelist(_wallet, _target, whitelistAfter); - emit AddedToWhitelist(address(_wallet), _target, uint64(whitelistAfter)); - } - - /** - * @dev Removes an address from the whitelist of a wallet. - * @param _wallet The target wallet. - * @param _target The address to remove. - */ - function removeFromWhitelist( - BaseWallet _wallet, - address _target - ) - external - onlyWalletOwner(_wallet) - onlyWhenUnlocked(_wallet) - { - require(isWhitelisted(_wallet, _target), "TT: target not whitelisted"); - transferStorage.setWhitelist(_wallet, _target, 0); - emit RemovedFromWhitelist(address(_wallet), _target); - } - - /** - * @dev Executes a pending transfer for a wallet. - * The destination address is automatically added to the whitelist. - * The method can be called by anyone to enable orchestration. - * @param _wallet The target wallet. - * @param _token The token of the pending transfer. - * @param _to The destination address of the pending transfer. - * @param _amount The amount of token to transfer of the pending transfer. - * @param _block The block at which the pending transfer was created. - */ - function executePendingTransfer( - BaseWallet _wallet, - address _token, - address _to, - uint _amount, - bytes memory _data, - uint _block - ) - public - onlyWhenUnlocked(_wallet) - { - bytes32 id = keccak256(abi.encodePacked(_token, _to, _amount, _data, _block)); - uint executeAfter = configs[address(_wallet)].pendingTransfers[id]; - uint executeBefore = executeAfter.add(securityWindow); - require(executeAfter <= now && now <= executeBefore, "TT: outside of the execution window"); - removePendingTransfer(_wallet, id); - if(_token == ETH_TOKEN) { - transferETH(_wallet, _to, _amount, _data); - } - else { - transferERC20(_wallet, _token, _to, _amount, _data); - } - emit PendingTransferExecuted(address(_wallet), id); - } - - /** - * @dev Cancels a pending transfer for a wallet. - * @param _wallet The target wallet. - * @param _id the pending transfer Id. - */ - function cancelPendingTransfer( - BaseWallet _wallet, - bytes32 _id - ) - public - onlyWalletOwner(_wallet) - onlyWhenUnlocked(_wallet) - { - require(configs[address(_wallet)].pendingTransfers[_id] > 0, "TT: unknown pending transfer"); - removePendingTransfer(_wallet, _id); - emit PendingTransferCanceled(address(_wallet), _id); - } - - /** - * @dev Lets the owner of a wallet change its global limit. - * The limit is expressed in ETH. Changes to the limit take 24 hours. - * @param _wallet The target wallet. - * @param _newLimit The new limit. - */ - function changeLimit(BaseWallet _wallet, uint256 _newLimit) public onlyWalletOwner(_wallet) onlyWhenUnlocked(_wallet) { - changeLimit(_wallet, _newLimit, securityPeriod); - } - - /** - * @dev Convenience method to disable the limit - * The limit is disabled by setting it to an arbitrary large value. - * @param _wallet The target wallet. - */ - function disableLimit(BaseWallet _wallet) external onlyWalletOwner(_wallet) onlyWhenUnlocked(_wallet) { - changeLimit(_wallet, LIMIT_DISABLED, securityPeriod); - } - - /** - * @dev Checks if an address is whitelisted for a wallet. - * @param _wallet The target wallet. - * @param _target The address. - * @return true if the address is whitelisted. - */ - function isWhitelisted(BaseWallet _wallet, address _target) public view returns (bool _isWhitelisted) { - uint whitelistAfter = transferStorage.getWhitelist(_wallet, _target); - // solium-disable-next-line security/no-block-members - return whitelistAfter > 0 && whitelistAfter < now; - } - - /** - * @dev Gets the info of a pending transfer for a wallet. - * @param _wallet The target wallet. - * @param _id The pending transfer Id. - * @return the epoch time at which the pending transfer can be executed. - */ - function getPendingTransfer(BaseWallet _wallet, bytes32 _id) external view returns (uint64 _executeAfter) { - _executeAfter = uint64(configs[address(_wallet)].pendingTransfers[_id]); - } - - // *************** Internal Functions ********************* // - - /** - * @dev Helper method to transfer ETH for a wallet. - * @param _wallet The target wallet. - * @param _to The recipient. - * @param _value The amount of ETH to transfer - * @param _data The data to *log* with the transfer. - */ - function transferETH(BaseWallet _wallet, address _to, uint256 _value, bytes memory _data) internal { - _wallet.invoke(_to, _value, EMPTY_BYTES); - emit Transfer(address(_wallet), ETH_TOKEN, _value, _to, _data); - } - - /** - * @dev Helper method to transfer ERC20 for a wallet. - * @param _wallet The target wallet. - * @param _token The ERC20 address. - * @param _to The recipient. - * @param _value The amount of token to transfer - * @param _data The data to pass with the trnasfer. - */ - function transferERC20(BaseWallet _wallet, address _token, address _to, uint256 _value, bytes memory _data) internal { - bytes memory methodData = abi.encodeWithSignature("transfer(address,uint256)", _to, _value); - _wallet.invoke(_token, 0, methodData); - emit Transfer(address(_wallet), _token, _value, _to, _data); - } - - /** - * @dev Creates a new pending transfer for a wallet. - * @param _wallet The target wallet. - * @param _token The token for the transfer. - * @param _to The recipient for the transfer. - * @param _amount The amount of token to transfer. - * @param _data The data associated to the transfer. - * @return the identifier for the new pending transfer. - */ - function addPendingTransfer(BaseWallet _wallet, address _token, address _to, uint _amount, bytes memory _data) internal returns (bytes32) { - bytes32 id = keccak256(abi.encodePacked(_token, _to, _amount, _data, block.number)); - uint executeAfter = now.add(securityPeriod); - configs[address(_wallet)].pendingTransfers[id] = executeAfter; - emit PendingTransferCreated(address(_wallet), id, executeAfter, _token, _to, _amount, _data); - } - - /** - * @dev Removes an existing pending transfer. - * @param _wallet The target wallet - * @param _id The id of the transfer to remove. - */ - function removePendingTransfer(BaseWallet _wallet, bytes32 _id) internal { - delete configs[address(_wallet)].pendingTransfers[_id]; - } - - // *************** Implementation of RelayerModule methods ********************* // - - // Overrides refund to add the refund in the daily limit. - function refund(BaseWallet _wallet, uint _gasUsed, uint _gasPrice, uint _gasLimit, uint _signatures, address _relayer) internal { - // 21000 (transaction) + 7620 (execution of refund) + 7324 (execution of updateDailySpent) + 672 to log the event + _gasUsed - uint256 amount = 36616 + _gasUsed; - if(_gasPrice > 0 && _signatures > 0 && amount <= _gasLimit) { - if(_gasPrice > tx.gasprice) { - amount = amount * tx.gasprice; - } - else { - amount = amount * _gasPrice; - } - updateDailySpent(_wallet, uint128(getCurrentLimit(_wallet)), amount); - _wallet.invoke(_relayer, amount, ""); - } - } - - // Overrides verifyRefund to add the refund in the daily limit. - function verifyRefund(BaseWallet _wallet, uint _gasUsed, uint _gasPrice, uint _signatures) internal view returns (bool) { - if(_gasPrice > 0 && _signatures > 0 && ( - address(_wallet).balance < _gasUsed * _gasPrice - || isWithinDailyLimit(_wallet, getCurrentLimit(_wallet), _gasUsed * _gasPrice) == false - || _wallet.authorised(address(_wallet)) == false - )) - { - return false; - } - return true; - } - - // Overrides to use the incremental nonce and save some gas - function checkAndUpdateUniqueness(BaseWallet _wallet, uint256 _nonce, bytes32 /* _signHash */) internal returns (bool) { - return checkAndUpdateNonce(_wallet, _nonce); - } - - function validateSignatures( - BaseWallet _wallet, - bytes memory /* _data */, - bytes32 _signHash, - bytes memory _signatures - ) - internal - view - returns (bool) - { - address signer = recoverSigner(_signHash, _signatures, 0); - return isOwner(_wallet, signer); // "TT: signer must be owner" - } - - function getRequiredSignatures(BaseWallet /* _wallet */, bytes memory _data) internal view returns (uint256) { - bytes4 methodId = functionPrefix(_data); - if (methodId == EXECUTE_PENDING_PREFIX) { - return 0; - } - return 1; - } -} \ No newline at end of file diff --git a/deployment/5_deploy_modules.js b/deployment/5_deploy_modules.js index ce9844884..63951af7b 100644 --- a/deployment/5_deploy_modules.js +++ b/deployment/5_deploy_modules.js @@ -92,7 +92,7 @@ const deploy = async (network) => { config.settings.securityPeriod || 0, config.settings.securityWindow || 0, config.settings.defaultLimit || "1000000000000000000", - ["test", "staging", "prod"].includes(network) ? config.modules.TokenTransfer : "0x0000000000000000000000000000000000000000", + "0x0000000000000000000000000000000000000000", ); // Deploy the TokenExchanger module const TokenExchangerWrapper = await deployer.deploy( diff --git a/deployment/7_upgrade_1_6.js b/deployment/7_upgrade_1_6.js index bf60d720b..2a3547d84 100644 --- a/deployment/7_upgrade_1_6.js +++ b/deployment/7_upgrade_1_6.js @@ -12,11 +12,12 @@ const TokenPriceProvider = require("../build/TokenPriceProvider"); const MakerRegistry = require("../build/MakerRegistry"); const ScdMcdMigration = require("../build/ScdMcdMigration"); const MakerV2Manager = require("../build/MakerV2Manager"); +const TransferManager = require("../build/TransferManager"); const utils = require("../utils/utilities.js"); const TARGET_VERSION = "1.6.0"; -const MODULES_TO_ENABLE = ["ApprovedTransfer", "RecoveryManager", "MakerV2Manager"]; +const MODULES_TO_ENABLE = ["ApprovedTransfer", "RecoveryManager", "MakerV2Manager", "TransferManager"]; const MODULES_TO_DISABLE = ["UniswapManager"]; const BACKWARD_COMPATIBILITY = 3; @@ -101,6 +102,20 @@ const deploy = async (network) => { ); newModuleWrappers.push(MakerV2ManagerWrapper); + const TransferManagerWrapper = await deployer.deploy( + TransferManager, + {}, + config.contracts.ModuleRegistry, + config.modules.TransferStorage, + config.modules.GuardianStorage, + config.contracts.TokenPriceProvider, + config.settings.securityPeriod || 0, + config.settings.securityWindow || 0, + config.settings.defaultLimit || "1000000000000000000", + ["test", "staging", "prod"].includes(network) ? config.modules.TransferManager : "0x0000000000000000000000000000000000000000", + ); + newModuleWrappers.push(TransferManagerWrapper); + // ///////////////////////////////////////////////// // Update config and Upload ABIs // ///////////////////////////////////////////////// @@ -109,6 +124,7 @@ const deploy = async (network) => { ApprovedTransfer: ApprovedTransferWrapper.contractAddress, RecoveryManager: RecoveryManagerWrapper.contractAddress, MakerV2Manager: MakerV2ManagerWrapper.contractAddress, + TransferManager: TransferManagerWrapper.contractAddress, }); configurator.updateInfrastructureAddresses({ @@ -123,6 +139,7 @@ const deploy = async (network) => { abiUploader.upload(ApprovedTransferWrapper, "modules"), abiUploader.upload(RecoveryManagerWrapper, "modules"), abiUploader.upload(MakerV2ManagerWrapper, "modules"), + abiUploader.upload(TransferManagerWrapper, "modules"), abiUploader.upload(MakerRegistryWrapper, "contracts"), ]); diff --git a/package.json b/package.json index 6aac071dd..e69849703 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "test": "npx etherlime test --skip-compilation", "ctest": "npm run compile && npm run test", "provision:lib:artefacts": "bash ./scripts/provision_lib_artefacts.sh", - "test:coverage": "bash ./scripts/provision_lib_artefacts.sh & npx etherlime coverage --solcVersion 0.5.4 && istanbul check-coverage --statements 71 --branches 64 --functions 72 --lines 71", + "test:coverage": "bash ./scripts/provision_lib_artefacts.sh & npx etherlime coverage --solcVersion 0.5.4 && istanbul check-coverage --statements 70 --branches 62 --functions 73 --lines 70", "lint:contracts": "npx ethlint --dir .", "lint:contracts:staged": "bash ./scripts/ethlint.sh", "test:deployment": "./scripts/deploy.sh ganache `seq 1 7`", diff --git a/test/approvedTransfer.js b/test/approvedTransfer.js index 8479cf929..fff7aa197 100644 --- a/test/approvedTransfer.js +++ b/test/approvedTransfer.js @@ -5,7 +5,7 @@ const Wallet = require("../build/BaseWallet"); const Registry = require("../build/ModuleRegistry"); const GuardianStorage = require("../build/GuardianStorage"); const GuardianManager = require("../build/GuardianManager"); -const TransferModule = require("../build/ApprovedTransfer"); +const ApprovedTransfer = require("../build/ApprovedTransfer"); const KyberNetwork = require("../build/KyberNetworkTest"); const TokenPriceProvider = require("../build/TokenPriceProvider"); const ERC20 = require("../build/TestERC20"); @@ -38,7 +38,7 @@ describe("Approved Transfer", function () { let deployer; let wallet; let guardianManager; - let transferModule; + let approvedTransfer; let priceProvider; let kyber; let erc20; @@ -52,11 +52,11 @@ describe("Approved Transfer", function () { priceProvider = await deployer.deploy(TokenPriceProvider, {}, kyber.contractAddress); await priceProvider.addManager(infrastructure.address); guardianManager = await deployer.deploy(GuardianManager, {}, registry.contractAddress, guardianStorage.contractAddress, 24, 12); - transferModule = await deployer.deploy(TransferModule, {}, registry.contractAddress, guardianStorage.contractAddress); + approvedTransfer = await deployer.deploy(ApprovedTransfer, {}, registry.contractAddress, guardianStorage.contractAddress); }); beforeEach(async () => { wallet = await deployer.deploy(Wallet); - await wallet.init(owner.address, [transferModule.contractAddress, guardianManager.contractAddress]); + await wallet.init(owner.address, [approvedTransfer.contractAddress, guardianManager.contractAddress]); erc20 = await deployer.deploy(ERC20, {}, [infrastructure.address, wallet.contractAddress], 10000000, DECIMALS); // TOKN contract with 10M tokens (5M TOKN for wallet and 5M TOKN for account[0]) await kyber.addToken(erc20.contractAddress, KYBER_RATE, DECIMALS); await priceProvider.syncPrice(erc20.contractAddress); @@ -96,7 +96,7 @@ describe("Approved Transfer", function () { async function transferToken(_token, _signers) { const to = recipient.address; const before = _token === ETH_TOKEN ? await deployer.provider.getBalance(to) : await erc20.balanceOf(to); - await manager.relay(transferModule, "transferToken", + await manager.relay(approvedTransfer, "transferToken", [wallet.contractAddress, _token, to, amountToTransfer, ZERO_BYTES32], wallet, _signers); const after = _token === ETH_TOKEN ? await deployer.provider.getBalance(to) : await erc20.balanceOf(to); assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the amount"); @@ -105,7 +105,7 @@ describe("Approved Transfer", function () { async function expectFailingTransferToken(_token, _signers, _reason) { await assert.revertWith( manager.relay( - transferModule, + approvedTransfer, "transferToken", [wallet.contractAddress, _token, recipient.address, amountToTransfer, ZERO_BYTES32], wallet, @@ -225,7 +225,7 @@ describe("Approved Transfer", function () { const before = await deployer.provider.getBalance(contract.contractAddress); const newState = parseInt((await contract.state()).toString(), 10) + 1; const dataToTransfer = contract.contract.interface.functions.setState.encode([newState]); - await manager.relay(transferModule, "callContract", + await manager.relay(approvedTransfer, "callContract", [wallet.contractAddress, contract.contractAddress, amountToTransfer, dataToTransfer], wallet, _signers); const after = await deployer.provider.getBalance(contract.contractAddress); assert.equal(after.sub(before).toNumber(), amountToTransfer, "should have transfered the ETH amount"); @@ -239,7 +239,7 @@ describe("Approved Transfer", function () { }); it("should not be able to call the wallet itself", async () => { - const txReceipt = await manager.relay(transferModule, "callContract", + const txReceipt = await manager.relay(approvedTransfer, "callContract", [wallet.contractAddress, wallet.contractAddress, amountToTransfer, ethers.constants.HashZero], wallet, [owner, ...sortWalletByAddress([guardian1, guardian2])]); @@ -263,17 +263,19 @@ describe("Approved Transfer", function () { describe("Invalid Target", () => { async function expectFailingApproveTokenAndCallContract(target) { const invalidData = contract.contract.interface.functions.setStateAndPayToken.encode([2, erc20.contractAddress, amountToApprove]); - const txReceipt = await manager.relay(transferModule, "approveTokenAndCallContract", + const txReceipt = await manager.relay(approvedTransfer, "approveTokenAndCallContract", [wallet.contractAddress, erc20.contractAddress, wallet.contractAddress, amountToApprove, target.contractAddress, invalidData], wallet, [owner, ...sortWalletByAddress([guardian1, guardian2])]); const success = parseRelayReceipt(txReceipt); assert.isNotOk(success, "approveTokenAndCall should fail when target contract is invalid"); } + it("should revert when target contract is the wallet", async () => { await expectFailingApproveTokenAndCallContract(wallet); }); + it("should revert when target contract is an authorised module", async () => { - await expectFailingApproveTokenAndCallContract(transferModule); + await expectFailingApproveTokenAndCallContract(approvedTransfer); }); }); @@ -286,7 +288,7 @@ describe("Approved Transfer", function () { ); const before = await erc20.balanceOf(contract.contractAddress); await manager.relay( - transferModule, + approvedTransfer, "approveTokenAndCallContract", [wallet.contractAddress, erc20.contractAddress, _consumerAddress, amountToApprove, contract.contractAddress, data], wallet, @@ -296,17 +298,38 @@ describe("Approved Transfer", function () { assert.equal(after.sub(before).toNumber(), amountToApprove, "should have approved and transfered the token amount"); assert.equal((await contract.state()).toNumber(), newState, "the state of the external contract should have been changed"); } + it("should approve token for a spender then call a contract with 3 guardians, spender = contract", async () => { await approveTokenAndCallContract([owner, ...sortWalletByAddress([guardian1, guardian2])]); await approveTokenAndCallContract([owner, ...sortWalletByAddress([guardian1, guardian3])]); await approveTokenAndCallContract([owner, ...sortWalletByAddress([guardian2, guardian3])]); }); + it("should approve token for a spender then call a contract with 3 guardians, spender != contract", async () => { const consumer = await contract.tokenConsumer(); await approveTokenAndCallContract([owner, ...sortWalletByAddress([guardian1, guardian2])], consumer); await approveTokenAndCallContract([owner, ...sortWalletByAddress([guardian1, guardian3])], consumer); await approveTokenAndCallContract([owner, ...sortWalletByAddress([guardian2, guardian3])], consumer); }); + + it("should restore the original approved amount", async () => { + const consumer = await contract.tokenConsumer(); + const allowanceBefore = await erc20.allowance(wallet.contractAddress, consumer); + const balanceBefore = await erc20.balanceOf(contract.contractAddress); + + const dataToTransfer = contract.contract.interface.functions + .setStateAndPayTokenWithConsumer.encode([2, erc20.contractAddress, amountToApprove]); + await manager.relay(approvedTransfer, "approveTokenAndCallContract", + [wallet.contractAddress, erc20.contractAddress, consumer, amountToApprove, contract.contractAddress, dataToTransfer], + wallet, [owner, ...sortWalletByAddress([guardian1, guardian2])]); + + const balanceAfter = await erc20.balanceOf(contract.contractAddress); + assert.equal(balanceAfter.sub(balanceBefore).toNumber(), amountToApprove, "should have approved and transfered the token amount"); + assert.equal((await contract.state()).toNumber(), 2, "the state of the external contract should have been changed"); + + const allowanceAfter = await erc20.allowance(wallet.contractAddress, consumer); + assert.equal(allowanceAfter.toNumber(), allowanceBefore.toNumber()); + }); }); }); }); diff --git a/test/transferManager.js b/test/transferManager.js index b06871c74..583d82184 100644 --- a/test/transferManager.js +++ b/test/transferManager.js @@ -6,7 +6,7 @@ const Registry = require("../build/ModuleRegistry"); const TransferStorage = require("../build/TransferStorage"); const GuardianStorage = require("../build/GuardianStorage"); const TransferModule = require("../build/TransferManager"); -const OldTransferModule = require("../build/LegacyTokenTransfer"); +const LegacyTransferManager = require("../build/LegacyTransferManager"); const KyberNetwork = require("../build/KyberNetworkTest"); const TokenPriceProvider = require("../build/TokenPriceProvider"); const ERC20 = require("../build/TestERC20"); @@ -53,14 +53,17 @@ describe("TransferManager", function () { priceProvider = await deployer.deploy(TokenPriceProvider, {}, kyber.contractAddress); transferStorage = await deployer.deploy(TransferStorage); guardianStorage = await deployer.deploy(GuardianStorage); - previousTransferModule = await deployer.deploy(OldTransferModule, {}, + + previousTransferModule = await deployer.deploy(LegacyTransferManager, {}, registry.contractAddress, transferStorage.contractAddress, guardianStorage.contractAddress, priceProvider.contractAddress, SECURITY_PERIOD, SECURITY_WINDOW, - ETH_LIMIT); + ETH_LIMIT, + ethers.constants.AddressZero); + transferModule = await deployer.deploy(TransferModule, {}, registry.contractAddress, transferStorage.contractAddress, @@ -70,6 +73,7 @@ describe("TransferManager", function () { SECURITY_WINDOW, ETH_LIMIT, previousTransferModule.contractAddress); + await registry.registerModule(transferModule.contractAddress, ethers.utils.formatBytes32String("TransferModule")); }); @@ -486,11 +490,12 @@ describe("TransferManager", function () { }); async function doApproveTokenAndCallContract({ - signer = owner, amount, state, relayed = false, + signer = owner, consumer = contract.contractAddress, amount, state, relayed = false, }) { - const dataToTransfer = contract.contract.interface.functions.setStateAndPayToken.encode([state, erc20.contractAddress, amount]); + const fun = consumer === contract.contractAddress ? "setStateAndPayToken" : "setStateAndPayTokenWithConsumer"; + const dataToTransfer = contract.contract.interface.functions[fun].encode([state, erc20.contractAddress, amount]); const unspentBefore = await transferModule.getDailyUnspent(wallet.contractAddress); - const params = [wallet.contractAddress, erc20.contractAddress, contract.contractAddress, amount, dataToTransfer]; + const params = [wallet.contractAddress, erc20.contractAddress, consumer, amount, contract.contractAddress, dataToTransfer]; let txReceipt; if (relayed) { txReceipt = await manager.relay(transferModule, "approveTokenAndCallContract", params, wallet, [signer]); @@ -498,7 +503,7 @@ describe("TransferManager", function () { const tx = await transferModule.from(signer).approveTokenAndCallContract(...params); txReceipt = await transferModule.verboseWaitForTransaction(tx); } - assert.isTrue(await utils.hasEvent(txReceipt, transferModule, "CalledContract"), "should have generated CalledContract event"); + assert.isTrue(await utils.hasEvent(txReceipt, transferModule, "ApprovedAndCalledContract"), "should have generated CalledContract event"); const unspentAfter = await transferModule.getDailyUnspent(wallet.contractAddress); const amountInEth = await priceProvider.getEtherValue(amount, erc20.contractAddress); @@ -519,14 +524,56 @@ describe("TransferManager", function () { await doApproveTokenAndCallContract({ amount: 10, state: 3, relayed: true }); }); - it("should approve the token and call the contract when under the existing approved amount", async () => { + it("should restore existing approved amount after call", async () => { await transferModule.from(owner).approveToken(wallet.contractAddress, erc20.contractAddress, contract.contractAddress, 10); - const dataToTransfer = contract.contract.interface.functions.setStateAndPayToken.encode([3, erc20.contractAddress, 1]); - await transferModule.from(owner) - .approveTokenAndCallContract(wallet.contractAddress, erc20.contractAddress, contract.contractAddress, 1, dataToTransfer); + const dataToTransfer = contract.contract.interface.functions.setStateAndPayToken.encode([3, erc20.contractAddress, 5]); + await transferModule.from(owner).approveTokenAndCallContract( + wallet.contractAddress, + erc20.contractAddress, + contract.contractAddress, + 5, + contract.contractAddress, + dataToTransfer, + ); const approval = await erc20.allowance(wallet.contractAddress, contract.contractAddress); - // No approval updates were done to the initial approval of 10, then the call succeeded with 1, leaving 9 - assert.equal(approval.toNumber(), 9); + + // Initial approval of 10 is restored, after approving and spending 5 + assert.equal(approval.toNumber(), 10); + + const erc20Balance = await erc20.balanceOf(contract.contractAddress); + assert.equal(erc20Balance.toNumber(), 5, "the contract should have transfered the tokens"); + }); + + it("should be able to spend less than approved in call", async () => { + await transferModule.from(owner).approveToken(wallet.contractAddress, erc20.contractAddress, contract.contractAddress, 10); + const dataToTransfer = contract.contract.interface.functions.setStateAndPayToken.encode([3, erc20.contractAddress, 4]); + await transferModule.from(owner).approveTokenAndCallContract( + wallet.contractAddress, + erc20.contractAddress, + contract.contractAddress, + 5, + contract.contractAddress, + dataToTransfer, + ); + const approval = await erc20.allowance(wallet.contractAddress, contract.contractAddress); + // Initial approval of 10 is restored, after approving and spending 4 + assert.equal(approval.toNumber(), 10); + + const erc20Balance = await erc20.balanceOf(contract.contractAddress); + assert.equal(erc20Balance.toNumber(), 4, "the contract should have transfered the tokens"); + }); + + it("should not be able to spend more than approved in call", async () => { + await transferModule.from(owner).approveToken(wallet.contractAddress, erc20.contractAddress, contract.contractAddress, 10); + const dataToTransfer = contract.contract.interface.functions.setStateAndPayToken.encode([3, erc20.contractAddress, 6]); + await assert.revertWith(transferModule.from(owner).approveTokenAndCallContract( + wallet.contractAddress, + erc20.contractAddress, + contract.contractAddress, + 5, + contract.contractAddress, + dataToTransfer, + ), "BT: insufficient amount for call"); }); it("should approve the token and call the contract when the token is above the limit and the contract is whitelisted ", async () => { @@ -535,6 +582,11 @@ describe("TransferManager", function () { await doApproveTokenAndCallContract({ amount: ETH_LIMIT + 10000, state: 6 }); }); + it("should approve the token and call the contract when contract to call is different to token spender", async () => { + const consumer = await contract.tokenConsumer(); + await doApproveTokenAndCallContract({ amount: 10, state: 3, spender: consumer }); + }); + it("should fail to approve the token and call the contract when the token is above the daily limit ", async () => { try { await doApproveTokenAndCallContract({ amount: ETH_LIMIT + 10000, state: 6 }); diff --git a/utils/config/ganache.json b/utils/config/ganache.json index 4a43a86a5..7cff3a38d 100644 --- a/utils/config/ganache.json +++ b/utils/config/ganache.json @@ -1 +1 @@ -{"ENS":{"deployOwnRegistry":true,"ensRegistry":"0x9d6CfEc16741f08991654Bc36d1045A1D241F29e","domain":"argent.xyz"},"backend":{"accounts":["0xD9995BAE12FEe327256FFec1e3184d492bD94C31"]},"multisig":{"owners":["0xD9995BAE12FEe327256FFec1e3184d492bD94C31"],"threshold":1,"autosign":true},"settings":{"deployer":{"type":"ganache"},"lockPeriod":480,"recoveryPeriod":480,"securityPeriod":240,"securityWindow":240,"feeRatio":15,"defaultLimit":"1000000000000000000"},"Kyber":{"deployOwn":true,"contract":"0x81A1198d28a7491F1257B22DaB444a73ABCf9298"},"CryptoKitties":{"contract":"0x0000000000000000000000000000000000000000"},"defi":{"maker":{"deployOwn":true,"tub":"0x0000000000000000000000000000000000000000","pot":"0x0000000000000000000000000000000000000000","jug":"0x0000000000000000000000000000000000000000","migration":"0x09ce60Bd50333a737e03db957bd7f845b08DB095"},"uniswap":{"deployOwn":true,"factory":"0xb5d7Ef06dd742b62d71eb2CaBa47d320b4D2A578"},"compound":{"comptroller":"0x0000000000000000000000000000000000000000","markets":{"0x0000000000000000000000000000000000000000":"0x0000000000000000000000000000000000000000"}}},"contracts":{"MultiSigWallet":"0xE6192099D3489211a92be2645aDe339ad4998a8c","WalletFactory":"0x0d326c46DCb8a85242C80d3c2A1a426CD2328401","ENSResolver":"0xD629AF06612B631c5b370055dA1cB3FB46A1c392","ENSManager":"0xb7358663Ed8f0E0aF7b05482AF9E8016DE3b554F","TokenPriceProvider":"0x1C2878ABd6a189D5449e797D735545A9C08d9234","ModuleRegistry":"0x0c1bCf5DF51D47ebD980cBb870dF35dCE283e771","BaseWallet":"0xa9442f66E6bEF32c3d196B71e943406e4722326b","CompoundRegistry":"0x93e33b71aF68a10ACd863A5DF1D5D81719Ac15B2","MakerRegistry":"0x4576CA92833f8A1E99A33E1595cce9E5349dD19d"},"modules":{"GuardianStorage":"0xE9c77f070a5671fA32381ac826036eE3FDF5Ad56","TransferStorage":"0x73C62c291621e20799CA5D6Bf7b114F678ebE9Cb","GuardianManager":"0xd85C69EA74b1DBaBb40Eea240E5Cd2Ce4426856F","LockManager":"0x0eA33b7525189c49e8Da46AD99278480811fcA64","RecoveryManager":"0xC07f212AaEeF5Dc948BBC5302cB46D9D799417Ec","ApprovedTransfer":"0x84c01919c85CE8c0a43F7A6b95FC5B3cf6230D0a","TokenTransfer":"0x101b60626A8073271fE2eFE59FB8fD74c108c2eC","TokenExchanger":"0x88A8e4a07fed276B6EeDB33449bfE9b0b5448B1F","NftTransfer":"0xD41288d45765e93ef63F58ac11CAa271BdA2bC21","MakerManager":"0x8A90941A8fB0B386eEEbFb2E1E96b54Af59ec29B","TransferManager":"0x39dCCe15d1a2c17c61B757C34cbe6aacF813Edc1","CompoundManager":"0xDCD324973848077F310E884CA7207b1Ecc67B493","MakerV2Manager":"0xFc31F3f732e9F1293ca3eb587871691bC8b28b82"},"gitCommit":"be87cf3131b90214644372a9331d20e64622ce03"} \ No newline at end of file +{"ENS":{"deployOwnRegistry":true,"ensRegistry":"0x9d6CfEc16741f08991654Bc36d1045A1D241F29e","domain":"argent.xyz"},"backend":{"accounts":["0xD9995BAE12FEe327256FFec1e3184d492bD94C31"]},"multisig":{"owners":["0xD9995BAE12FEe327256FFec1e3184d492bD94C31"],"threshold":1,"autosign":true},"settings":{"deployer":{"type":"ganache"},"lockPeriod":480,"recoveryPeriod":480,"securityPeriod":240,"securityWindow":240,"feeRatio":15,"defaultLimit":"1000000000000000000"},"Kyber":{"deployOwn":true,"contract":"0x81A1198d28a7491F1257B22DaB444a73ABCf9298"},"CryptoKitties":{"contract":"0x0000000000000000000000000000000000000000"},"defi":{"maker":{"deployOwn":true,"tub":"0x0000000000000000000000000000000000000000","pot":"0x0000000000000000000000000000000000000000","jug":"0x0000000000000000000000000000000000000000","migration":"0x09ce60Bd50333a737e03db957bd7f845b08DB095"},"uniswap":{"deployOwn":true,"factory":"0xb5d7Ef06dd742b62d71eb2CaBa47d320b4D2A578"},"compound":{"comptroller":"0x0000000000000000000000000000000000000000","markets":{"0x0000000000000000000000000000000000000000":"0x0000000000000000000000000000000000000000"}}},"contracts":{"MultiSigWallet":"0xE6192099D3489211a92be2645aDe339ad4998a8c","WalletFactory":"0x0d326c46DCb8a85242C80d3c2A1a426CD2328401","ENSResolver":"0xD629AF06612B631c5b370055dA1cB3FB46A1c392","ENSManager":"0xb7358663Ed8f0E0aF7b05482AF9E8016DE3b554F","TokenPriceProvider":"0x1C2878ABd6a189D5449e797D735545A9C08d9234","ModuleRegistry":"0x0c1bCf5DF51D47ebD980cBb870dF35dCE283e771","BaseWallet":"0xa9442f66E6bEF32c3d196B71e943406e4722326b","CompoundRegistry":"0x93e33b71aF68a10ACd863A5DF1D5D81719Ac15B2","MakerRegistry":"0x4576CA92833f8A1E99A33E1595cce9E5349dD19d"},"modules":{"GuardianStorage":"0xE9c77f070a5671fA32381ac826036eE3FDF5Ad56","TransferStorage":"0x73C62c291621e20799CA5D6Bf7b114F678ebE9Cb","GuardianManager":"0xd85C69EA74b1DBaBb40Eea240E5Cd2Ce4426856F","LockManager":"0x0eA33b7525189c49e8Da46AD99278480811fcA64","RecoveryManager":"0xC07f212AaEeF5Dc948BBC5302cB46D9D799417Ec","ApprovedTransfer":"0x84c01919c85CE8c0a43F7A6b95FC5B3cf6230D0a","TokenExchanger":"0x88A8e4a07fed276B6EeDB33449bfE9b0b5448B1F","NftTransfer":"0xD41288d45765e93ef63F58ac11CAa271BdA2bC21","MakerManager":"0x8A90941A8fB0B386eEEbFb2E1E96b54Af59ec29B","TransferManager":"0x39dCCe15d1a2c17c61B757C34cbe6aacF813Edc1","CompoundManager":"0xDCD324973848077F310E884CA7207b1Ecc67B493","MakerV2Manager":"0xFc31F3f732e9F1293ca3eb587871691bC8b28b82"},"gitCommit":"be87cf3131b90214644372a9331d20e64622ce03"} \ No newline at end of file