diff --git a/.solcover.js b/.solcover.js index 706360613..38a69357e 100644 --- a/.solcover.js +++ b/.solcover.js @@ -3,6 +3,7 @@ const skipFiles = [ 'test', 'acl/ACLSyntaxSugar.sol', 'common/DepositableStorage.sol', // Used in tests that send ETH + 'common/SafeERC20.sol', // solidity-coverage fails on assembly if (https://github.com/sc-forks/solidity-coverage/issues/287) 'common/UnstructuredStorage.sol' // Used in tests that send ETH ] diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 000000000..51b282139 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,72 @@ +# Contributing to aragonOS + +:tada: Thank you for being interested in contributing to aragonOS! :tada: + +Feel welcome and read the following sections in order to know how to ask questions and how to work on something. + +There are many ways to contribute, from writing tutorials or blog posts, improving the documentation, submitting bug reports and feature requests or writing code which can be incorporated into the project. + +All members of our community are expected to follow our [Code of Conduct](https://wiki.aragon.org/documentation/Code_of_Conduct/). Please make sure you are welcoming and friendly in all of our spaces. + +## Your first contribution + +Unsure where to begin contributing to aragonOS? + +You can start with a [Good First Issue](https://github.com/aragon/aragonOS/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22) + +> Good first issues are usually for small features, additional tests, spelling / grammar fixes, formatting changes, or other clean up. + +Start small, pick a subject you care about, are familiar with, or want to learn. + +If you're not already familiar with git or Github, here are a couple of friendly tutorials: [First Contributions](https://github.com/firstcontributions/first-contributions), [Open Source Guide](https://opensource.guide/), and [How to Contribute to an Open Source Project on GitHub](https://egghead.io/series/how-to-contribute-to-an-open-source-project-on-github). + +## How to file an issue or report a bug + +If you see a problem, you can report it in our [issue tracker](https://github.com/aragon/aragonOS/issues). + +Please take a quick look to see if the issue doesn't already exist before filing yours. + +Do your best to include as many details as needed in order for someone else to fix the problem and resolve the issue. + +#### If you find a security vulnerability, do NOT open an issue. Email security@aragon.org instead. + +In order to determine whether you are dealing with a security issue, ask yourself these two questions: + +- Can I access or steal something that's not mine, or access something I shouldn't have access to? +- Can I disable something for other people? + +If the answer to either of those two questions are "yes", then you're probably dealing with a security issue. Note that even if you answer "no" to both questions, you may still be dealing with a security issue, so if you're unsure, please send a email. + +#### A [bug bounty program](https://wiki.aragon.org/dev/bug_bounty/) is available for rewarding contributors who find security vulnerabilities with payouts up to $50,000. + +## Fixing issues + +1. [Find an issue](https://github.com/aragon/aragonOS/issues) that you are interested in. + - You may want to ask on the issue or on Aragon Chat's [#dev channel](https://aragon.chat/channel/dev) if anyone has already started working on the issue. +1. Fork and clone a local copy of the repository. +1. Make the appropriate changes for the issue you are trying to address or the feature that you want to add. + - Make sure to add tests! +1. Push the changes to the remote repository. +1. Submit a pull request in Github, explaining any changes and further questions you may have. +1. Wait for the pull request to be reviewed. +1. Make changes to the pull request if the maintainer recommends them. +1. Celebrate your success after your pull request is merged! + +It's OK if your pull request is not perfect (no pull request is). +The reviewer will be able to help you fix any problems and improve it! + +You can also edit a page directly through your browser by clicking the "EDIT" link in the top-right corner of any page and then clicking the pencil icon in the github copy of the page. + +## Styleguide and development processes + +We generally follow [Solidity's style guide](https://solidity.readthedocs.io/en/v0.4.24/style-guide.html) and have set up [Ethlint](https://github.com/duaraghav8/Ethlint) to automatically lint the project. + +Due to the sensitive nature of Solidity, usually at least two reviewers are required before merging any pull request with code changes. + +### Licensing + +aragonOS is generally meant to be used as a library by developers but includes core components that are not generally useful to extend. Any interfaces or contracts meant to be used by other developers are licensed as MIT and have their Solidity pragmas left unpinned. All other contracts are licensed as GPL-3 and are pinned to a specific Solidity version. + +## Community + +If you need help, please reach out to Aragon core contributors and community members in the Aragon Chat [#dev](https://aragon.chat/channel/dev) [#dev-help](https://aragon.chat/channel/dev-help) channels. We'd love to hear from you and know what you're working on! diff --git a/contracts/acl/ACL.sol b/contracts/acl/ACL.sol index c9c62d770..109cbe748 100644 --- a/contracts/acl/ACL.sol +++ b/contracts/acl/ACL.sol @@ -2,37 +2,20 @@ pragma solidity 0.4.24; import "../apps/AragonApp.sol"; import "../common/TimeHelpers.sol"; -import "./ACLSyntaxSugar.sol"; +import "./ACLHelpers.sol"; +import "./ACLParams.sol"; import "./IACL.sol"; import "./IACLOracle.sol"; /* solium-disable function-order */ // Allow public initialize() to be first -contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers { +contract ACL is IACL, TimeHelpers, AragonApp, ACLParams, ACLHelpers { /* Hardcoded constants to save gas bytes32 public constant CREATE_PERMISSIONS_ROLE = keccak256("CREATE_PERMISSIONS_ROLE"); */ bytes32 public constant CREATE_PERMISSIONS_ROLE = 0x0b719b33c83b8e5d300c521cb8b54ae9bd933996a14bef8c2f4e0285d2d2400a; - enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } // op types - - struct Param { - uint8 id; - uint8 op; - uint240 value; // even though value is an uint240 it can store addresses - // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal - // op and id take less than 1 byte each so it can be kept in 1 sstore - } - - uint8 internal constant BLOCK_NUMBER_PARAM_ID = 200; - uint8 internal constant TIMESTAMP_PARAM_ID = 201; - // 202 is unused - uint8 internal constant ORACLE_PARAM_ID = 203; - uint8 internal constant LOGIC_OP_PARAM_ID = 204; - uint8 internal constant PARAM_VALUE_PARAM_ID = 205; - // TODO: Add execution times param type? - /* Hardcoded constant to save gas bytes32 public constant EMPTY_PARAM_HASH = keccak256(uint256(0)); */ diff --git a/contracts/acl/ACLHelpers.sol b/contracts/acl/ACLHelpers.sol new file mode 100644 index 000000000..829297e79 --- /dev/null +++ b/contracts/acl/ACLHelpers.sol @@ -0,0 +1,46 @@ +/* + * SPDX-License-Identitifer: MIT + */ + +pragma solidity ^0.4.24; + +import "./ACLParams.sol"; + + +contract ACLHelpers is ACLParams { + function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { + return uint8(_x >> (8 * 30)); + } + + function decodeParamId(uint256 _x) internal pure returns (uint8 b) { + return uint8(_x >> (8 * 31)); + } + + function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { + a = uint32(_x); + b = uint32(_x >> (8 * 4)); + c = uint32(_x >> (8 * 8)); + } + + function encodeParams(Param[] params) internal pure returns (uint256[]) { + uint256[] memory encodedParams = new uint256[](params.length); + + for (uint i = 0; i < params.length; i++) { + encodedParams[i] = encodeParam(params[i]); + } + + return encodedParams; + } + + function encodeParam(Param param) internal pure returns (uint256) { + return uint256(param.id) << 248 | uint256(param.op) << 240 | param.value; + } + + function encodeOperator(uint256 param1, uint256 param2) internal pure returns (uint240) { + return uint240(param1 + (param2 << 32) + (0 << 64)); + } + + function encodeIfElse(uint256 condition, uint256 successParam, uint256 failureParam) internal pure returns (uint240) { + return uint240(condition + (successParam << 32) + (failureParam << 64)); + } +} diff --git a/contracts/acl/ACLParams.sol b/contracts/acl/ACLParams.sol new file mode 100644 index 000000000..479baeaa6 --- /dev/null +++ b/contracts/acl/ACLParams.sol @@ -0,0 +1,28 @@ +/* + * SPDX-License-Identitifer: MIT + */ + +pragma solidity ^0.4.24; + + +contract ACLParams { + // Op types + enum Op { NONE, EQ, NEQ, GT, LT, GTE, LTE, RET, NOT, AND, OR, XOR, IF_ELSE } + + // Op IDs + uint8 internal constant BLOCK_NUMBER_PARAM_ID = 200; + uint8 internal constant TIMESTAMP_PARAM_ID = 201; + // 202 is unused + uint8 internal constant ORACLE_PARAM_ID = 203; + uint8 internal constant LOGIC_OP_PARAM_ID = 204; + uint8 internal constant PARAM_VALUE_PARAM_ID = 205; + // TODO: Add execution times param type? + + struct Param { + uint8 id; + uint8 op; + uint240 value; // even though value is an uint240 it can store addresses + // in the case of 32 byte hashes losing 2 bytes precision isn't a huge deal + // op and id take less than 1 byte each so it can be kept in 1 sstore + } +} diff --git a/contracts/acl/ACLSyntaxSugar.sol b/contracts/acl/ACLSyntaxSugar.sol index 38ca29fdc..428fd4af4 100644 --- a/contracts/acl/ACLSyntaxSugar.sol +++ b/contracts/acl/ACLSyntaxSugar.sol @@ -6,7 +6,9 @@ pragma solidity ^0.4.24; contract ACLSyntaxSugar { - function arr() internal pure returns (uint256[]) {} + function arr() internal pure returns (uint256[]) { + // solium-disable-previous-line no-empty-blocks + } function arr(bytes32 _a) internal pure returns (uint256[] r) { return arr(uint256(_a)); @@ -83,20 +85,3 @@ contract ACLSyntaxSugar { r[4] = _e; } } - - -contract ACLHelpers { - function decodeParamOp(uint256 _x) internal pure returns (uint8 b) { - return uint8(_x >> (8 * 30)); - } - - function decodeParamId(uint256 _x) internal pure returns (uint8 b) { - return uint8(_x >> (8 * 31)); - } - - function decodeParamsList(uint256 _x) internal pure returns (uint32 a, uint32 b, uint32 c) { - a = uint32(_x); - b = uint32(_x >> (8 * 4)); - c = uint32(_x >> (8 * 8)); - } -} diff --git a/contracts/apm/APMRegistry.sol b/contracts/apm/APMRegistry.sol index 031014145..b457f4b70 100644 --- a/contracts/apm/APMRegistry.sol +++ b/contracts/apm/APMRegistry.sol @@ -31,6 +31,8 @@ contract APMRegistry is AragonApp, AppProxyFactory, APMInternalAppNames { /** * NEEDS CREATE_NAME_ROLE and POINT_ROOTNODE_ROLE permissions on registrar + * @dev Initialize can only be called once. It saves the block number in which it was initialized + * @notice Initialize this APMRegistry instance and set `_registrar` as the ENS subdomain registrar * @param _registrar ENSSubdomainRegistrar instance that holds registry root node ownership */ function initialize(ENSSubdomainRegistrar _registrar) public onlyInit { diff --git a/contracts/apm/Repo.sol b/contracts/apm/Repo.sol index 095aab21b..30b54d430 100644 --- a/contracts/apm/Repo.sol +++ b/contracts/apm/Repo.sol @@ -30,7 +30,7 @@ contract Repo is AragonApp { /** * @dev Initialize can only be called once. It saves the block number in which it was initialized. - * @notice Initializes a Repo to be usable + * @notice Initialize this Repo */ function initialize() public onlyInit { initialized(); diff --git a/contracts/apps/AppProxyUpgradeable.sol b/contracts/apps/AppProxyUpgradeable.sol index 60d4cfa0c..84ca19578 100644 --- a/contracts/apps/AppProxyUpgradeable.sol +++ b/contracts/apps/AppProxyUpgradeable.sol @@ -14,7 +14,7 @@ contract AppProxyUpgradeable is AppProxyBase { AppProxyBase(_kernel, _appId, _initializePayload) public // solium-disable-line visibility-first { - + // solium-disable-previous-line no-empty-blocks } /** diff --git a/contracts/common/IVaultRecoverable.sol b/contracts/common/IVaultRecoverable.sol index f747b9a0f..3c8c3676c 100644 --- a/contracts/common/IVaultRecoverable.sol +++ b/contracts/common/IVaultRecoverable.sol @@ -6,6 +6,8 @@ pragma solidity ^0.4.24; interface IVaultRecoverable { + event RecoverToVault(address indexed vault, address indexed token, uint256 amount); + function transferToVault(address token) external; function allowRecoverability(address token) external view returns (bool); diff --git a/contracts/common/SafeERC20.sol b/contracts/common/SafeERC20.sol new file mode 100644 index 000000000..fa11c0921 --- /dev/null +++ b/contracts/common/SafeERC20.sol @@ -0,0 +1,156 @@ +// Inspired by AdEx (https://github.com/AdExNetwork/adex-protocol-eth/blob/b9df617829661a7518ee10f4cb6c4108659dd6d5/contracts/libs/SafeERC20.sol) +// and 0x (https://github.com/0xProject/0x-monorepo/blob/737d1dc54d72872e24abce5a1dbe1b66d35fa21a/contracts/protocol/contracts/protocol/AssetProxy/ERC20Proxy.sol#L143) + +pragma solidity ^0.4.24; + +import "../lib/token/ERC20.sol"; + + +library SafeERC20 { + // Before 0.5, solidity has a mismatch between `address.transfer()` and `token.transfer()`: + // https://github.com/ethereum/solidity/issues/3544 + bytes4 private constant TRANSFER_SELECTOR = 0xa9059cbb; + + string private constant ERROR_TOKEN_BALANCE_REVERTED = "SAFE_ERC_20_BALANCE_REVERTED"; + string private constant ERROR_TOKEN_ALLOWANCE_REVERTED = "SAFE_ERC_20_ALLOWANCE_REVERTED"; + + function invokeAndCheckSuccess(address _addr, bytes memory _calldata) + private + returns (bool) + { + bool ret; + assembly { + let ptr := mload(0x40) // free memory pointer + + let success := call( + gas, // forward all gas + _addr, // address + 0, // no value + add(_calldata, 0x20), // calldata start + mload(_calldata), // calldata length + ptr, // write output over free memory + 0x20 // uint256 return + ) + + if gt(success, 0) { + // Check number of bytes returned from last function call + switch returndatasize + + // No bytes returned: assume success + case 0 { + ret := 1 + } + + // 32 bytes returned: check if non-zero + case 0x20 { + // Only return success if returned data was true + // Already have output in ptr + ret := eq(mload(ptr), 1) + } + + // Not sure what was returned: don't mark as success + default { } + } + } + return ret; + } + + function staticInvoke(address _addr, bytes memory _calldata) + private + view + returns (bool, uint256) + { + bool success; + uint256 ret; + assembly { + let ptr := mload(0x40) // free memory pointer + + success := staticcall( + gas, // forward all gas + _addr, // address + add(_calldata, 0x20), // calldata start + mload(_calldata), // calldata length + ptr, // write output over free memory + 0x20 // uint256 return + ) + + if gt(success, 0) { + ret := mload(ptr) + } + } + return (success, ret); + } + + /** + * @dev Same as a standards-compliant ERC20.transfer() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeTransfer(ERC20 _token, address _to, uint256 _amount) internal returns (bool) { + bytes memory transferCallData = abi.encodeWithSelector( + TRANSFER_SELECTOR, + _to, + _amount + ); + return invokeAndCheckSuccess(_token, transferCallData); + } + + /** + * @dev Same as a standards-compliant ERC20.transferFrom() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeTransferFrom(ERC20 _token, address _from, address _to, uint256 _amount) internal returns (bool) { + bytes memory transferFromCallData = abi.encodeWithSelector( + _token.transferFrom.selector, + _from, + _to, + _amount + ); + return invokeAndCheckSuccess(_token, transferFromCallData); + } + + /** + * @dev Same as a standards-compliant ERC20.approve() that never reverts (returns false). + * Note that this makes an external call to the token. + */ + function safeApprove(ERC20 _token, address _spender, uint256 _amount) internal returns (bool) { + bytes memory approveCallData = abi.encodeWithSelector( + _token.approve.selector, + _spender, + _amount + ); + return invokeAndCheckSuccess(_token, approveCallData); + } + + /** + * @dev Static call into ERC20.balanceOf(). + * Reverts if the call fails for some reason (should never fail). + */ + function staticBalanceOf(ERC20 _token, address _owner) internal view returns (uint256) { + bytes memory balanceOfCallData = abi.encodeWithSelector( + _token.balanceOf.selector, + _owner + ); + + (bool success, uint256 tokenBalance) = staticInvoke(_token, balanceOfCallData); + require(success, ERROR_TOKEN_BALANCE_REVERTED); + + return tokenBalance; + } + + /** + * @dev Static call into ERC20.allowance(). + * Reverts if the call fails for some reason (should never fail). + */ + function staticAllowance(ERC20 _token, address _owner, address _spender) internal view returns (uint256) { + bytes memory allowanceCallData = abi.encodeWithSelector( + _token.allowance.selector, + _owner, + _spender + ); + + (bool success, uint256 allowance) = staticInvoke(_token, allowanceCallData); + require(success, ERROR_TOKEN_ALLOWANCE_REVERTED); + + return allowance; + } +} diff --git a/contracts/common/VaultRecoverable.sol b/contracts/common/VaultRecoverable.sol index c298d66b4..74aacd41d 100644 --- a/contracts/common/VaultRecoverable.sol +++ b/contracts/common/VaultRecoverable.sol @@ -8,11 +8,15 @@ import "../lib/token/ERC20.sol"; import "./EtherTokenConstant.sol"; import "./IsContract.sol"; import "./IVaultRecoverable.sol"; +import "./SafeERC20.sol"; contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract { + using SafeERC20 for ERC20; + string private constant ERROR_DISALLOWED = "RECOVER_DISALLOWED"; string private constant ERROR_VAULT_NOT_CONTRACT = "RECOVER_VAULT_NOT_CONTRACT"; + string private constant ERROR_TOKEN_TRANSFER_FAILED = "RECOVER_TOKEN_TRANSFER_FAILED"; /** * @notice Send funds to recovery Vault. This contract should never receive funds, @@ -24,12 +28,17 @@ contract VaultRecoverable is IVaultRecoverable, EtherTokenConstant, IsContract { address vault = getRecoveryVault(); require(isContract(vault), ERROR_VAULT_NOT_CONTRACT); + uint256 balance; if (_token == ETH) { - vault.transfer(address(this).balance); + balance = address(this).balance; + vault.transfer(balance); } else { - uint256 amount = ERC20(_token).balanceOf(this); - ERC20(_token).transfer(vault, amount); + ERC20 token = ERC20(_token); + balance = token.staticBalanceOf(this); + require(token.safeTransfer(vault, balance), ERROR_TOKEN_TRANSFER_FAILED); } + + emit RecoverToVault(vault, _token, balance); } /** diff --git a/contracts/ens/ENSSubdomainRegistrar.sol b/contracts/ens/ENSSubdomainRegistrar.sol index 671985e19..9fa88113b 100644 --- a/contracts/ens/ENSSubdomainRegistrar.sol +++ b/contracts/ens/ENSSubdomainRegistrar.sol @@ -29,6 +29,12 @@ contract ENSSubdomainRegistrar is AragonApp, ENSConstants { event NewName(bytes32 indexed node, bytes32 indexed label); event DeleteName(bytes32 indexed node, bytes32 indexed label); + /** + * @dev Initialize can only be called once. It saves the block number in which it was initialized. This contract must be the owner of the `_rootNode` node so that it can create subdomains. + * @notice Initialize this ENSSubdomainRegistrar instance with `_ens` as the root ENS registry and `_rootNode` as the node to allocate subdomains under + * @param _ens Address of ENS registry + * @param _rootNode Node to allocate subdomains under + */ function initialize(AbstractENS _ens, bytes32 _rootNode) public onlyInit { initialized(); @@ -39,15 +45,31 @@ contract ENSSubdomainRegistrar is AragonApp, ENSConstants { rootNode = _rootNode; } + /** + * @notice Create a new ENS subdomain record for `_label` and assign ownership to `_owner` + * @param _label Label of new subdomain + * @param _owner Owner of new subdomain + * @return node Hash of created node + */ function createName(bytes32 _label, address _owner) external auth(CREATE_NAME_ROLE) returns (bytes32 node) { return _createName(_label, _owner); } + /** + * @notice Create a new ENS subdomain record for `_label` that resolves to `_target` and is owned by this ENSSubdomainRegistrar + * @param _label Label of new subdomain + * @param _target Ethereum address this new subdomain label will point to + * @return node Hash of created node + */ function createNameAndPoint(bytes32 _label, address _target) external auth(CREATE_NAME_ROLE) returns (bytes32 node) { node = _createName(_label, this); _pointToResolverAndResolve(node, _target); } + /** + * @notice Deregister ENS subdomain record for `_label` + * @param _label Label of subdomain to deregister + */ function deleteName(bytes32 _label) external auth(DELETE_NAME_ROLE) { bytes32 node = getNodeForLabel(_label); @@ -65,6 +87,10 @@ contract ENSSubdomainRegistrar is AragonApp, ENSConstants { emit DeleteName(node, _label); } + /** + * @notice Resolve this ENSSubdomainRegistrar's root node to `_target` + * @param _target Ethereum address root node will point to + */ function pointRootNode(address _target) external auth(POINT_ROOTNODE_ROLE) { _pointToResolverAndResolve(rootNode, _target); } diff --git a/contracts/kernel/IKernel.sol b/contracts/kernel/IKernel.sol index 0775cec14..e1a2b40e5 100644 --- a/contracts/kernel/IKernel.sol +++ b/contracts/kernel/IKernel.sol @@ -8,10 +8,13 @@ import "../acl/IACL.sol"; import "../common/IVaultRecoverable.sol"; -// This should be an interface, but interfaces can't inherit yet :( -contract IKernel is IVaultRecoverable { +interface IKernelEvents { event SetApp(bytes32 indexed namespace, bytes32 indexed appId, address app); +} + +// This should be an interface, but interfaces can't inherit yet :( +contract IKernel is IKernelEvents, IVaultRecoverable { function acl() public view returns (IACL); function hasPermission(address who, address where, bytes32 what, bytes how) public view returns (bool); diff --git a/contracts/kernel/Kernel.sol b/contracts/kernel/Kernel.sol index 3698d520b..71c312d21 100644 --- a/contracts/kernel/Kernel.sol +++ b/contracts/kernel/Kernel.sol @@ -35,7 +35,7 @@ contract Kernel is IKernel, KernelStorage, KernelAppIds, KernelNamespaceConstant /** * @dev Initialize can only be called once. It saves the block number in which it was initialized. - * @notice Initializes a kernel instance along with its ACL and sets `_permissionsCreator` as the entity that can create other permissions + * @notice Initialize this kernel instance along with its ACL and set `_permissionsCreator` as the entity that can create other permissions * @param _baseAcl Address of base ACL app * @param _permissionsCreator Entity that will be given permission over createPermission */ diff --git a/contracts/kernel/KernelProxy.sol b/contracts/kernel/KernelProxy.sol index 8e30c1f2a..d298d8e28 100644 --- a/contracts/kernel/KernelProxy.sol +++ b/contracts/kernel/KernelProxy.sol @@ -7,7 +7,7 @@ import "../common/DepositableDelegateProxy.sol"; import "../common/IsContract.sol"; -contract KernelProxy is KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, DepositableDelegateProxy { +contract KernelProxy is IKernelEvents, KernelStorage, KernelAppIds, KernelNamespaceConstants, IsContract, DepositableDelegateProxy { /** * @dev KernelProxy is a proxy contract to a kernel implementation. The implementation * can update the reference, which effectively upgrades the contract @@ -16,6 +16,12 @@ contract KernelProxy is KernelStorage, KernelAppIds, KernelNamespaceConstants, I constructor(IKernel _kernelImpl) public { require(isContract(address(_kernelImpl))); apps[KERNEL_CORE_NAMESPACE][KERNEL_CORE_APP_ID] = _kernelImpl; + + // Note that emitting this event is important for verifying that a KernelProxy instance + // was never upgraded to a malicious Kernel logic contract over its lifespan. + // This starts the "chain of trust", that can be followed through later SetApp() events + // emitted during kernel upgrades. + emit SetApp(KERNEL_CORE_NAMESPACE, KERNEL_CORE_APP_ID, _kernelImpl); } /** diff --git a/contracts/test/TestACLHelpers.sol b/contracts/test/TestACLHelpers.sol new file mode 100644 index 000000000..49264d120 --- /dev/null +++ b/contracts/test/TestACLHelpers.sol @@ -0,0 +1,38 @@ +pragma solidity 0.4.24; + +import "./helpers/Assert.sol"; +import "../acl/ACLHelpers.sol"; + + +contract TestACLHelpers is ACLHelpers { + function testEncodeParam() public { + Param memory param = Param(2, uint8(Op.EQ), 5294967297); + + uint256 encodedParam = encodeParam(param); + + (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam); + + Assert.equal(uint256(param.id), uint256(id), "Encoded id is not equal"); + Assert.equal(uint256(param.op), uint256(op), "Encoded op is not equal"); + Assert.equal(uint256(param.value), uint256(value), "Encoded value is not equal"); + } + + function testEncodeParams() public { + Param[] memory params = new Param[](4); + + params[0] = Param(LOGIC_OP_PARAM_ID, uint8(Op.IF_ELSE), encodeIfElse(1, 2, 3)); + params[1] = Param(LOGIC_OP_PARAM_ID, uint8(Op.AND), encodeOperator(2, 3)); + params[2] = Param(2, uint8(Op.EQ), 1); + params[3] = Param(3, uint8(Op.NEQ), 2); + + uint256[] memory encodedParam = encodeParams(params); + + for (uint256 i = 0; i < 4; i++) { + (uint32 id, uint32 op, uint32 value) = decodeParamsList(encodedParam[i]); + + Assert.equal(uint256(params[i].id), uint256(id), "Encoded id is not equal"); + Assert.equal(uint256(params[i].op), uint256(op), "Encoded op is not equal"); + Assert.equal(uint256(params[i].value), uint256(value), "Encoded value is not equal"); + } + } +} diff --git a/contracts/test/TestACLInterpreter.sol b/contracts/test/TestACLInterpreter.sol index ca909e91e..c9056b24c 100644 --- a/contracts/test/TestACLInterpreter.sol +++ b/contracts/test/TestACLInterpreter.sol @@ -2,10 +2,10 @@ pragma solidity 0.4.24; import "../acl/ACL.sol"; import "./helpers/Assert.sol"; -import "./helpers/ACLHelper.sol"; +import "./helpers/ACLOracleHelper.sol"; -contract TestACLInterpreter is ACL, ACLHelper { +contract TestACLInterpreter is ACL { function testEqualityUint() public { // Assert param 0 is equal to 10, given that params are [10, 11] assertEval(arr(uint256(10), 11), 0, Op.EQ, 10, true); diff --git a/contracts/test/helpers/ACLHelper.sol b/contracts/test/helpers/ACLOracleHelper.sol similarity index 76% rename from contracts/test/helpers/ACLHelper.sol rename to contracts/test/helpers/ACLOracleHelper.sol index 404978572..74241755c 100644 --- a/contracts/test/helpers/ACLHelper.sol +++ b/contracts/test/helpers/ACLOracleHelper.sol @@ -3,17 +3,6 @@ pragma solidity 0.4.24; import "../../acl/IACLOracle.sol"; -contract ACLHelper { - function encodeOperator(uint256 param1, uint256 param2) internal pure returns (uint240) { - return encodeIfElse(param1, param2, 0); - } - - function encodeIfElse(uint256 condition, uint256 successParam, uint256 failureParam) internal pure returns (uint240) { - return uint240(condition + (successParam << 32) + (failureParam << 64)); - } -} - - contract AcceptOracle is IACLOracle { function canPerform(address, address, bytes32, uint256[]) external view returns (bool) { return true; diff --git a/contracts/test/mocks/APMRegistryFactoryMock.sol b/contracts/test/mocks/APMRegistryFactoryMock.sol index cadd24c0a..455948b59 100644 --- a/contracts/test/mocks/APMRegistryFactoryMock.sol +++ b/contracts/test/mocks/APMRegistryFactoryMock.sol @@ -1,31 +1,49 @@ pragma solidity 0.4.24; +import "../../apm/APMRegistry.sol"; +import "../../apm/Repo.sol"; +import "../../ens/ENSSubdomainRegistrar.sol"; + +import "../../factory/DAOFactory.sol"; +import "../../factory/ENSFactory.sol"; + // Mock that doesn't grant enough permissions -// external ENS +// Only usable with new ENS instance -import "../../factory/APMRegistryFactory.sol"; +contract APMRegistryFactoryMock is APMInternalAppNames { + DAOFactory public daoFactory; + APMRegistry public registryBase; + Repo public repoBase; + ENSSubdomainRegistrar public ensSubdomainRegistrarBase; + ENS public ens; -contract APMRegistryFactoryMock is APMRegistryFactory { constructor( DAOFactory _daoFactory, APMRegistry _registryBase, Repo _repoBase, ENSSubdomainRegistrar _ensSubBase, - ENS _ens, ENSFactory _ensFactory - ) - APMRegistryFactory(_daoFactory, _registryBase, _repoBase, _ensSubBase, _ens, _ensFactory) public {} - - function newAPM(bytes32, bytes32, address) public returns (APMRegistry) {} - - function newBadAPM(bytes32 tld, bytes32 label, address _root, bool withoutACL) public returns (APMRegistry) { - bytes32 node = keccak256(abi.encodePacked(tld, label)); + ) public + { + daoFactory = _daoFactory; + registryBase = _registryBase; + repoBase = _repoBase; + ensSubdomainRegistrarBase = _ensSubBase; + ens = _ensFactory.newENS(this); + } - // Assume it is the test ENS - if (ens.owner(node) != address(this)) { - // If we weren't in test ens and factory doesn't have ownership, will fail - ens.setSubnodeOwner(tld, label, this); - } + function newFailingAPM( + bytes32 _tld, + bytes32 _label, + address _root, + bool _withoutNameRole + ) + public + returns (APMRegistry) + { + // Set up ENS control + bytes32 node = keccak256(abi.encodePacked(_tld, _label)); + ens.setSubnodeOwner(_tld, _label, this); Kernel dao = daoFactory.newDAO(this); ACL acl = ACL(dao.acl()); @@ -55,34 +73,24 @@ contract APMRegistryFactoryMock is APMRegistryFactory { bytes32 repoAppId = keccak256(abi.encodePacked(node, keccak256(abi.encodePacked(REPO_APP_NAME)))); dao.setApp(dao.APP_BASES_NAMESPACE(), repoAppId, repoBase); - emit DeployAPM(node, apm); - // Grant permissions needed for APM on ENSSubdomainRegistrar + acl.createPermission(apm, ensSub, ensSub.POINT_ROOTNODE_ROLE(), _root); - if (withoutACL) { + // Don't grant all permissions needed for APM to initialize + if (_withoutNameRole) { acl.createPermission(apm, ensSub, ensSub.CREATE_NAME_ROLE(), _root); } - acl.createPermission(apm, ensSub, ensSub.POINT_ROOTNODE_ROLE(), _root); - - configureAPMPermissions(acl, apm, _root); - - // allow apm to create permissions for Repos in Kernel - bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE(); - - if (!withoutACL) { + if (!_withoutNameRole) { + bytes32 permRole = acl.CREATE_PERMISSIONS_ROLE(); acl.grantPermission(apm, acl, permRole); } - // Permission transition to _root - acl.setPermissionManager(_root, dao, dao.APP_MANAGER_ROLE()); - acl.revokePermission(this, acl, permRole); - acl.grantPermission(_root, acl, permRole); - acl.setPermissionManager(_root, acl, permRole); - // Initialize ens.setOwner(node, ensSub); ensSub.initialize(ens, node); + + // This should fail since we haven't given all required permissions apm.initialize(ensSub); return apm; diff --git a/contracts/test/mocks/SafeERC20Mock.sol b/contracts/test/mocks/SafeERC20Mock.sol new file mode 100644 index 000000000..92b660ced --- /dev/null +++ b/contracts/test/mocks/SafeERC20Mock.sol @@ -0,0 +1,36 @@ +pragma solidity 0.4.24; + +import "../../common/SafeERC20.sol"; +import "../../lib/token/ERC20.sol"; + + +contract SafeERC20Mock { + using SafeERC20 for ERC20; + event Result(bool result); + + function transfer(ERC20 token, address to, uint256 amount) external returns (bool) { + bool result = token.safeTransfer(to, amount); + emit Result(result); + return result; + } + + function transferFrom(ERC20 token, address from, address to, uint256 amount) external returns (bool) { + bool result = token.safeTransferFrom(from, to, amount); + emit Result(result); + return result; + } + + function approve(ERC20 token, address spender, uint256 amount) external returns (bool) { + bool result = token.safeApprove(spender, amount); + emit Result(result); + return result; + } + + function allowance(ERC20 token, address owner, address spender) external view returns (uint256) { + return token.staticAllowance(owner, spender); + } + + function balanceOf(ERC20 token, address owner) external view returns (uint256) { + return token.staticBalanceOf(owner); + } +} diff --git a/contracts/test/mocks/TokenMock.sol b/contracts/test/mocks/TokenMock.sol index 7442d80be..a93f777aa 100644 --- a/contracts/test/mocks/TokenMock.sol +++ b/contracts/test/mocks/TokenMock.sol @@ -1,4 +1,4 @@ -// Stripped from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol pragma solidity 0.4.24; @@ -8,14 +8,18 @@ import "../../lib/math/SafeMath.sol"; contract TokenMock { using SafeMath for uint256; mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; uint256 private totalSupply_; + bool private allowTransfer_; + event Approval(address indexed owner, address indexed spender, uint256 value); event Transfer(address indexed from, address indexed to, uint256 value); // Allow us to set the inital balance for an account on construction constructor(address initialAccount, uint256 initialBalance) public { balances[initialAccount] = initialBalance; totalSupply_ = initialBalance; + allowTransfer_ = true; } function totalSupply() public view returns (uint256) { return totalSupply_; } @@ -29,12 +33,31 @@ contract TokenMock { return balances[_owner]; } + /** + * @dev Function to check the amount of tokens that an owner allowed to a spender. + * @param _owner address The address which owns the funds. + * @param _spender address The address which will spend the funds. + * @return A uint256 specifying the amount of tokens still available for the spender. + */ + function allowance(address _owner, address _spender) public view returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @dev Set whether the token is transferable or not + * @param _allowTransfer Should token be transferable + */ + function setAllowTransfer(bool _allowTransfer) public { + allowTransfer_ = _allowTransfer; + } + /** * @dev Transfer token for a specified address * @param _to The address to transfer to. * @param _value The amount to be transferred. */ function transfer(address _to, uint256 _value) public returns (bool) { + require(allowTransfer_); require(_value <= balances[msg.sender]); require(_to != address(0)); @@ -43,4 +66,41 @@ contract TokenMock { emit Transfer(msg.sender, _to, _value); return true; } + + /** + * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender. + * Beware that changing an allowance with this method brings the risk that someone may use both the old + * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this + * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * @param _spender The address which will spend the funds. + * @param _value The amount of tokens to be spent. + */ + function approve(address _spender, uint256 _value) public returns (bool) { + // Assume we want to protect for the race condition + require(allowed[msg.sender][_spender] == 0); + + allowed[msg.sender][_spender] = _value; + emit Approval(msg.sender, _spender, _value); + return true; + } + + /** + * @dev Transfer tokens from one address to another + * @param _from address The address which you want to send tokens from + * @param _to address The address which you want to transfer to + * @param _value uint256 the amount of tokens to be transferred + */ + function transferFrom(address _from, address _to, uint256 _value) public returns (bool) { + require(allowTransfer_); + require(_value <= balances[_from]); + require(_value <= allowed[_from][msg.sender]); + require(_to != address(0)); + + balances[_from] = balances[_from].sub(_value); + balances[_to] = balances[_to].add(_value); + allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value); + emit Transfer(_from, _to, _value); + return true; + } } diff --git a/contracts/test/mocks/TokenReturnFalseMock.sol b/contracts/test/mocks/TokenReturnFalseMock.sol new file mode 100644 index 000000000..6011fc033 --- /dev/null +++ b/contracts/test/mocks/TokenReturnFalseMock.sol @@ -0,0 +1,110 @@ +// Standards compliant token that is returns false instead of reverting for +// `transfer()`, `transferFrom()`, and `approve(). +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol + +pragma solidity 0.4.24; + + +contract TokenReturnFalseMock { + mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; + uint256 private totalSupply_; + bool private allowTransfer_; + + event Approval(address indexed owner, address indexed spender, uint256 value); + event Transfer(address indexed from, address indexed to, uint256 value); + + // Allow us to set the inital balance for an account on construction + constructor(address initialAccount, uint256 initialBalance) public { + balances[initialAccount] = initialBalance; + totalSupply_ = initialBalance; + allowTransfer_ = true; + } + + function totalSupply() public view returns (uint256) { return totalSupply_; } + + /** + * @dev Gets the balance of the specified address. + * @param _owner The address to query the the balance of. + * @return An uint256 representing the amount owned by the passed address. + */ + function balanceOf(address _owner) public view returns (uint256) { + return balances[_owner]; + } + + /** + * @dev Function to check the amount of tokens that an owner allowed to a spender. + * @param _owner address The address which owns the funds. + * @param _spender address The address which will spend the funds. + * @return A uint256 specifying the amount of tokens still available for the spender. + */ + function allowance(address _owner, address _spender) public view returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @dev Set whether the token is transferable or not + * @param _allowTransfer Should token be transferable + */ + function setAllowTransfer(bool _allowTransfer) public { + allowTransfer_ = _allowTransfer; + } + + /** + * @dev Transfer token for a specified address + * @param _to The address to transfer to. + * @param _value The amount to be transferred. + */ + function transfer(address _to, uint256 _value) public returns (bool) { + if (!allowTransfer_ || _to == address(0) || _value > balances[msg.sender]) { + return false; + } + + balances[msg.sender] = balances[msg.sender] - _value; + balances[_to] = balances[_to] + _value; + emit Transfer(msg.sender, _to, _value); + return true; + } + + /** + * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender. + * Beware that changing an allowance with this method brings the risk that someone may use both the old + * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this + * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * @param _spender The address which will spend the funds. + * @param _value The amount of tokens to be spent. + */ + function approve(address _spender, uint256 _value) public returns (bool) { + // Assume we want to protect for the race condition + if (allowed[msg.sender][_spender] != 0) { + return false; + } + + allowed[msg.sender][_spender] = _value; + emit Approval(msg.sender, _spender, _value); + return true; + } + + /** + * @dev Transfer tokens from one address to another + * @param _from address The address which you want to send tokens from + * @param _to address The address which you want to transfer to + * @param _value uint256 the amount of tokens to be transferred + */ + function transferFrom(address _from, address _to, uint256 _value) public returns (bool) { + if (!allowTransfer_ || + _to == address(0) || + _value > balances[_from] || + _value > allowed[_from][msg.sender] + ) { + return false; + } + + balances[_from] = balances[_from] - _value; + balances[_to] = balances[_to] + _value; + allowed[_from][msg.sender] = allowed[_from][msg.sender] - _value; + emit Transfer(_from, _to, _value); + return true; + } +} diff --git a/contracts/test/mocks/TokenReturnMissingMock.sol b/contracts/test/mocks/TokenReturnMissingMock.sol new file mode 100644 index 000000000..8b339b1c8 --- /dev/null +++ b/contracts/test/mocks/TokenReturnMissingMock.sol @@ -0,0 +1,105 @@ +// Non-standards compliant token that is missing return values for +// `transfer()`, `transferFrom()`, and `approve(). +// Modified from https://github.com/OpenZeppelin/openzeppelin-solidity/blob/a9f910d34f0ab33a1ae5e714f69f9596a02b4d91/contracts/token/ERC20/StandardToken.sol + +pragma solidity 0.4.24; + +import "../../lib/math/SafeMath.sol"; + + +contract TokenReturnMissingMock { + using SafeMath for uint256; + mapping (address => uint256) private balances; + mapping (address => mapping (address => uint256)) private allowed; + uint256 private totalSupply_; + bool private allowTransfer_; + + event Approval(address indexed owner, address indexed spender, uint256 value); + event Transfer(address indexed from, address indexed to, uint256 value); + + // Allow us to set the inital balance for an account on construction + constructor(address initialAccount, uint256 initialBalance) public { + balances[initialAccount] = initialBalance; + totalSupply_ = initialBalance; + allowTransfer_ = true; + } + + function totalSupply() public view returns (uint256) { return totalSupply_; } + + /** + * @dev Gets the balance of the specified address. + * @param _owner The address to query the the balance of. + * @return An uint256 representing the amount owned by the passed address. + */ + function balanceOf(address _owner) public view returns (uint256) { + return balances[_owner]; + } + + /** + * @dev Set whether the token is transferable or not + * @param _allowTransfer Should token be transferable + */ + function setAllowTransfer(bool _allowTransfer) public { + allowTransfer_ = _allowTransfer; + } + + /** + * @dev Function to check the amount of tokens that an owner allowed to a spender. + * @param _owner address The address which owns the funds. + * @param _spender address The address which will spend the funds. + * @return A uint256 specifying the amount of tokens still available for the spender. + */ + function allowance(address _owner, address _spender) public view returns (uint256) { + return allowed[_owner][_spender]; + } + + /** + * @dev Transfer token for a specified address + * @param _to The address to transfer to. + * @param _value The amount to be transferred. + */ + function transfer(address _to, uint256 _value) public { + require(allowTransfer_); + require(_value <= balances[msg.sender]); + require(_to != address(0)); + + balances[msg.sender] = balances[msg.sender].sub(_value); + balances[_to] = balances[_to].add(_value); + emit Transfer(msg.sender, _to, _value); + } + + /** + * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender. + * Beware that changing an allowance with this method brings the risk that someone may use both the old + * and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this + * race condition is to first reduce the spender's allowance to 0 and set the desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * @param _spender The address which will spend the funds. + * @param _value The amount of tokens to be spent. + */ + function approve(address _spender, uint256 _value) public { + // Assume we want to protect for the race condition + require(allowed[msg.sender][_spender] == 0); + + allowed[msg.sender][_spender] = _value; + emit Approval(msg.sender, _spender, _value); + } + + /** + * @dev Transfer tokens from one address to another + * @param _from address The address which you want to send tokens from + * @param _to address The address which you want to transfer to + * @param _value uint256 the amount of tokens to be transferred + */ + function transferFrom(address _from, address _to, uint256 _value) public { + require(allowTransfer_); + require(_value <= balances[_from]); + require(_value <= allowed[_from][msg.sender]); + require(_to != address(0)); + + balances[_from] = balances[_from].sub(_value); + balances[_to] = balances[_to].add(_value); + allowed[_from][msg.sender] = allowed[_from][msg.sender].sub(_value); + emit Transfer(_from, _to, _value); + } +} diff --git a/package.json b/package.json index 9e02b7bb7..97e2b9e08 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@aragon/os", - "version": "4.0.1", + "version": "4.1.0-rc.1", "description": "Core contracts for Aragon", "scripts": { "compile": "truffle compile", @@ -37,13 +37,14 @@ "eth-ens-namehash": "^2.0.8", "eth-gas-reporter": "^0.1.1", "ethereumjs-abi": "^0.6.5", - "ganache-cli": "^6.2.0", + "ganache-cli": "6.2.3", "mocha-lcov-reporter": "^1.3.0", "solidity-coverage": "0.5.8", - "solium": "^1.1.8", + "solium": "^1.2.3", "truffle": "4.1.14", "truffle-bytecode-manager": "^1.1.1", "truffle-extract": "^1.2.1", + "web3-eth-abi": "1.0.0-beta.33", "web3-utils": "1.0.0-beta.33" }, "dependencies": { diff --git a/readme.md b/readme.md index 797883ad1..6ea126912 100644 --- a/readme.md +++ b/readme.md @@ -1,6 +1,6 @@ # aragonOS [![Travis branch](https://img.shields.io/travis/aragon/aragonOS/master.svg?style=for-the-badge)](https://travis-ci.org/aragon/aragonOS) [![Coveralls branch](https://img.shields.io/coveralls/aragon/aragonOS/master.svg?style=for-the-badge)](https://coveralls.io/github/aragon/aragonOS?branch=master) [![npm](https://img.shields.io/npm/v/@aragon/os.svg?style=for-the-badge)](https://www.npmjs.com/package/@aragon/os) -This repo contains Aragon's reference implementation for [aragonOS](https://hack.aragon.org/docs/aragonos-intro.html). +This repo contains Aragon's reference implementation of [aragonOS](https://hack.aragon.org/docs/aragonos-intro.html). #### 🚨 Security review status: bug bounty aragonOS 4 has undergone two independent professional security reviews, and the issues raised have been resolved. However there is a [bug bounty program](https://wiki.aragon.org/dev/bug_bounty/) for rewarding hackers who find security vulnerabilities. There is a bounty pool of $250,000 USD, you can find more information [here](https://wiki.aragon.org/dev/bug_bounty/). @@ -10,9 +10,9 @@ Don't be shy to contribute even the smallest tweak. Everyone will be especially ## Documentation -Visit the [Aragon Developer Portal](https://hack.aragon.org/docs/aragonos-intro.html) for in depth documentation on the [architecture](https://hack.aragon.org/docs/aragonos-ref.html) and different parts of the system. +Visit the [Aragon Developer Portal](https://hack.aragon.org/docs/aragonos-intro.html) for in-depth documentation on the [architecture](https://hack.aragon.org/docs/aragonos-ref.html) and different parts of the system. -## Installing aragonOS +## Developing aragonOS locally ```sh npm install @@ -32,14 +32,14 @@ OWNER=[APM owner address] ENS=[ENS registry address] npx truffle exec --network - `ENS`: If no ENS registry address is provided, it will deploy a dummy ENS instance to the network. If the ENS registry is provided, the name `aragonpm.eth` must be owned by the deployer account. - `OWNER`: The account that will be the initial owner of the APM registry -## Using aragonOS for making Aragon apps +## Adding aragonOS as a dependency to your Aragon app ``` npm i --save-dev @aragon/os ``` -Check the [Aragon Developer Portal](https://hack.aragon.org) for detailed documentation and tutorials on how to use aragonOS. +Check the [Aragon Developer Portal](https://hack.aragon.org) for detailed documentation and tutorials on how to use aragonOS to build an Aragon app. ## Contributing -For details about how to contribute you can check the [contributing guide](https://wiki.aragon.one/dev/aragonOS_how_to_contribute/) on the wiki. +For more details about contributing to aragonOS, please check the [contributing guide](./CONTRIBUTING.md). diff --git a/test/acl_helpers.js b/test/acl_helpers.js new file mode 100644 index 000000000..244f52787 --- /dev/null +++ b/test/acl_helpers.js @@ -0,0 +1,3 @@ +const runSolidityTest = require('./helpers/runSolidityTest') + +runSolidityTest('TestACLHelpers') \ No newline at end of file diff --git a/test/apm_registry.js b/test/apm_registry.js index 810428fc8..fa83e14a3 100644 --- a/test/apm_registry.js +++ b/test/apm_registry.js @@ -91,6 +91,17 @@ contract('APMRegistry', accounts => { assert.equal(await repo.getVersionsCount(), 2, 'should have created version') }) + it('fails to init with existing ENS deployment if not owner of tld', async () => { + const ensReceipt = await ensFactory.newENS(accounts[0]) + const ens2 = ENS.at(ensReceipt.logs.filter(l => l.event == 'DeployENS')[0].args.ens) + const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, '0x00') + + // Factory doesn't have ownership over 'eth' tld + await assertRevert(async () => { + await newFactory.newAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner) + }) + }) + it('fails to create empty repo name', async () => { return assertRevert(async () => { await registry.newRepo('', repoDev, { from: apmOwner }) @@ -103,7 +114,7 @@ contract('APMRegistry', accounts => { }) }) - context('creating test.aragonpm.eth repo', () => { + context('> Creating test.aragonpm.eth repo', () => { let repo = {} beforeEach(async () => { @@ -164,22 +175,22 @@ contract('APMRegistry', accounts => { }) }) - context('APMRegistry created with lacking permissions', () => { + context('> Created with missing permissions', () => { let apmFactoryMock before(async () => { - apmFactoryMock = await APMRegistryFactoryMock.new(daoFactory.address, ...baseAddrs, '0x0', ensFactory.address) + apmFactoryMock = await APMRegistryFactoryMock.new(daoFactory.address, ...baseAddrs, ensFactory.address) }) - it('fails if factory doesnt give permission to create permissions', async () => { - return assertRevert(async () => { - await apmFactoryMock.newBadAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, true) + it('fails if factory doesnt give permission to create names', async () => { + await assertRevert(async () => { + await apmFactoryMock.newFailingAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, true) }) }) - it('fails if factory doesnt give permission to create names', async () => { - return assertRevert(async () => { - await apmFactoryMock.newBadAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, false) + it('fails if factory doesnt give permission to create permissions', async () => { + await assertRevert(async () => { + await apmFactoryMock.newFailingAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner, false) }) }) }) diff --git a/test/ens_subdomains.js b/test/ens_subdomains.js index 708bc605b..62156bab2 100644 --- a/test/ens_subdomains.js +++ b/test/ens_subdomains.js @@ -10,14 +10,19 @@ const Kernel = artifacts.require('Kernel') const ACL = artifacts.require('ACL') const APMRegistry = artifacts.require('APMRegistry') +const AppProxyUpgradeable = artifacts.require('AppProxyUpgradeable') const ENSSubdomainRegistrar = artifacts.require('ENSSubdomainRegistrar') const Repo = artifacts.require('Repo') const APMRegistryFactory = artifacts.require('APMRegistryFactory') const DAOFactory = artifacts.require('DAOFactory') +const EMPTY_BYTES = '0x' + // Using APMFactory in order to reuse it contract('ENSSubdomainRegistrar', accounts => { - let baseDeployed, apmFactory, ensFactory, daoFactory, ens, registrar + let baseDeployed, apmFactory, ensFactory, dao, daoFactory, ens, registrar + let APP_BASES_NAMESPACE + const ensOwner = accounts[0] const apmOwner = accounts[1] const repoDev = accounts[2] @@ -38,6 +43,8 @@ contract('ENSSubdomainRegistrar', accounts => { const kernelBase = await Kernel.new(true) // petrify immediately const aclBase = await ACL.new() daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, '0x00') + + APP_BASES_NAMESPACE = await kernelBase.APP_BASES_NAMESPACE() }) beforeEach(async () => { @@ -49,7 +56,7 @@ contract('ENSSubdomainRegistrar', accounts => { const apmAddr = receipt.logs.filter(l => l.event == 'DeployAPM')[0].args.apm const registry = APMRegistry.at(apmAddr) - const dao = Kernel.at(await registry.kernel()) + dao = Kernel.at(await registry.kernel()) const acl = ACL.at(await dao.acl()) registrar = ENSSubdomainRegistrar.at(await registry.registrar()) @@ -99,11 +106,12 @@ contract('ENSSubdomainRegistrar', accounts => { }) it('fails if initializing without rootnode ownership', async () => { - const reg = await ENSSubdomainRegistrar.new() const ens = await ENS.new() + const newRegProxy = await AppProxyUpgradeable.new(dao.address, namehash('apm-enssub.aragonpm.eth'), EMPTY_BYTES) + const newReg = ENSSubdomainRegistrar.at(newRegProxy.address) - return assertRevert(async () => { - await reg.initialize(ens.address, holanode) + await assertRevert(async () => { + await newReg.initialize(ens.address, holanode) }) }) }) diff --git a/test/evm_script.js b/test/evm_script.js index f3c5bd4e2..9615e02c2 100644 --- a/test/evm_script.js +++ b/test/evm_script.js @@ -109,6 +109,7 @@ contract('EVM Script', accounts => { assertEvent(receipt, 'DisableExecutor') assert.isFalse(executorEntry[1], "executor should now be disabled") + assert.equal(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return zero addr') }) it('can re-enable an executor', async () => { @@ -117,12 +118,14 @@ contract('EVM Script', accounts => { await reg.disableScriptExecutor(newExecutorId, { from: boss }) let executorEntry = await reg.executors(newExecutorId) assert.isFalse(executorEntry[1], "executor should now be disabled") + assert.equal(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return zero addr') const receipt = await reg.enableScriptExecutor(newExecutorId, { from: boss }) executorEntry = await reg.executors(newExecutorId) assertEvent(receipt, 'EnableExecutor') assert.isTrue(executorEntry[1], "executor should now be re-enabled") + assert.notEqual(await reg.getScriptExecutor(`0x0000000${newExecutorId}`), ZERO_ADDR, 'getting disabled executor should return non-zero addr') }) it('fails to add a new executor without the correct permissions', async () => { @@ -153,6 +156,13 @@ contract('EVM Script', accounts => { }) }) + it('fails to enable a non-existent executor', async () => { + await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) + await assertRevert(async () => { + await reg.enableScriptExecutor(newExecutorId + 1, { from: boss }) + }) + }) + it('fails to disable an already disabled executor', async () => { await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) await reg.disableScriptExecutor(newExecutorId, { from: boss }) @@ -326,37 +336,23 @@ contract('EVM Script', accounts => { }) }) - context('registry', () => { - it('can be disabled', async () => { + context('> Registry actions', () => { + it("can't be executed once disabled", async () => { await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - const receipt = await reg.disableScriptExecutor(1, { from: boss }) - const isEnabled = (await reg.executors(1))[1] // enabled flag is second in struct + await reg.disableScriptExecutor(1, { from: boss }) - assertEvent(receipt, 'DisableExecutor') - assert.equal(await reg.getScriptExecutor('0x00000001'), ZERO_ADDR, 'getting disabled executor should return zero addr') - assert.isFalse(isEnabled, 'executor should be disabled') return assertRevert(async () => { await executorApp.execute(encodeCallScript([])) }) }) it('can be re-enabled', async () => { - let isEnabled await acl.createPermission(boss, reg.address, await reg.REGISTRY_MANAGER_ROLE(), boss, { from: boss }) - // First, disable the executor + // Disable then re-enable the executor await reg.disableScriptExecutor(1, { from: boss }) - isEnabled = (await reg.executors(1))[1] // enabled flag is second in struct - assert.equal(await reg.getScriptExecutor('0x00000001'), ZERO_ADDR, 'getting disabled executor should return zero addr') - assert.isFalse(isEnabled, 'executor should be disabled') - - // Then re-enable it - const receipt = await reg.enableScriptExecutor(1, { from: boss }) - isEnabled = (await reg.executors(1))[1] // enabled flag is second in struct + await reg.enableScriptExecutor(1, { from: boss }) - assertEvent(receipt, 'EnableExecutor') - assert.notEqual(await reg.getScriptExecutor('0x00000001'), ZERO_ADDR, 'getting enabled executor should be non-zero addr') - assert.isTrue(isEnabled, 'executor should be enabled') await executorApp.execute(encodeCallScript([])) }) }) diff --git a/test/helpers/decodeEvent.js b/test/helpers/decodeEvent.js new file mode 100644 index 000000000..0a8a2d7e8 --- /dev/null +++ b/test/helpers/decodeEvent.js @@ -0,0 +1,14 @@ +const abi = require('web3-eth-abi') + +module.exports = { + decodeEventsOfType: (receipt, contractAbi, eventName) => { + const eventAbi = contractAbi.filter(abi => abi.name === eventName && abi.type === 'event')[0] + const eventSignature = abi.encodeEventSignature(eventAbi) + const eventLogs = receipt.logs.filter(l => l.topics[0] === eventSignature) + return eventLogs.map(log => { + log.event = abi.name + log.args = abi.decodeLog(eventAbi.inputs, log.data, log.topics.slice(1)) + return log + }) + } +} diff --git a/test/kernel_upgrade.js b/test/kernel_upgrade.js index 22f4494d1..db0d908b7 100644 --- a/test/kernel_upgrade.js +++ b/test/kernel_upgrade.js @@ -1,4 +1,5 @@ const { assertRevert } = require('./helpers/assertThrow') +const { decodeEventsOfType } = require('./helpers/decodeEvent') const ACL = artifacts.require('ACL') const Kernel = artifacts.require('Kernel') @@ -47,6 +48,19 @@ contract('Kernel upgrade', accounts => { assert.equal(proxyType, UPGRADEABLE, "Proxy type should be 2 (upgradeable)") }) + it('emits SetApp event', async () => { + const kernelProxy = await KernelProxy.new(kernelBase.address) + const receipt = web3.eth.getTransactionReceipt(kernelProxy.transactionHash) + + const setAppLogs = decodeEventsOfType(receipt, kernelProxy.abi, 'SetApp') + assert.equal(setAppLogs.length, 1) + + const setAppArgs = setAppLogs[0].args + assert.equal(setAppArgs.namespace, CORE_NAMESPACE, 'Kernel namespace should match') + assert.equal(setAppArgs.appId, KERNEL_APP_ID, 'Kernel app id should match') + assert.equal(setAppArgs.app.toLowerCase(), kernelBase.address.toLowerCase(), 'Kernel base address should match') + }) + it('fails to create a KernelProxy if the base is 0', async () => { return assertRevert(async () => { await KernelProxy.new(ZERO_ADDR) diff --git a/test/proxy_recover_funds.js b/test/recovery_to_vault.js similarity index 58% rename from test/proxy_recover_funds.js rename to test/recovery_to_vault.js index 94bdddd9a..a92714540 100644 --- a/test/proxy_recover_funds.js +++ b/test/recovery_to_vault.js @@ -1,3 +1,4 @@ +const assertEvent = require('./helpers/assertEvent') const { assertRevert } = require('./helpers/assertThrow') const { skipCoverage } = require('./helpers/coverage') const { getBalance } = require('./helpers/web3') @@ -14,6 +15,8 @@ const AppStubDepositable = artifacts.require('AppStubDepositable') const AppStubConditionalRecovery = artifacts.require('AppStubConditionalRecovery') const EtherTokenConstantMock = artifacts.require('EtherTokenConstantMock') const TokenMock = artifacts.require('TokenMock') +const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock') +const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock') const VaultMock = artifacts.require('VaultMock') const KernelDepositableMock = artifacts.require('KernelDepositableMock') @@ -22,34 +25,80 @@ const getEvent = (receipt, event, arg) => { return receipt.logs.filter(l => l.ev const APP_ID = hash('stub.aragonpm.test') const SEND_ETH_GAS = 31000 // 21k base tx cost + 10k limit on depositable proxies -contract('Proxy funds', accounts => { +contract('Recovery to vault', accounts => { let aclBase, appBase, appConditionalRecoveryBase let APP_ADDR_NAMESPACE, ETH const permissionsRoot = accounts[0] // Helpers - const recoverEth = async (target, vault) => { + const recoverEth = async ({ shouldFail, target, vault }) => { const amount = 1 const initialBalance = await getBalance(target.address) const initialVaultBalance = await getBalance(vault.address) const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) - assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) - await target.transferToVault(ETH) - assert.equal((await getBalance(target.address)).valueOf(), 0) - assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount).valueOf()) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') + + const recoverAction = () => target.transferToVault(ETH) + + if (shouldFail) { + await assertRevert(recoverAction) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount), 'Target balance should be same as before') + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance, 'Vault balance should should be same as before') + } else { + const recoverReceipt = await recoverAction() + assert.equal((await getBalance(target.address)).valueOf(), 0, 'Target balance should be 0') + assert.equal((await getBalance(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount), 'Vault balance should include recovered amount') + + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'vault'), vault.address, 'RecoverToVault event should have correct vault') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'token'), ETH, 'RecoverToVault event should have correct token') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'amount'), amount, 'RecoverToVault event should have correct amount') + assertEvent(recoverReceipt, 'RecoverToVault', 1) + } } - const recoverTokens = async (target, vault) => { + const recoverTokens = async ({ shouldFail, tokenContract, target, vault }) => { const amount = 1 - const token = await TokenMock.new(accounts[0], 1000) + const token = await tokenContract.new(accounts[0], 1000) const initialBalance = await token.balanceOf(target.address) const initialVaultBalance = await token.balanceOf(vault.address) await token.transfer(target.address, amount) - assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount)) - await target.transferToVault(token.address) - assert.equal((await token.balanceOf(target.address)).valueOf(), 0) - assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount).valueOf()) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') + + const recoverAction = () => target.transferToVault(token.address) + + if (shouldFail) { + await assertRevert(recoverAction) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target balance should be same as before') + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance, 'Vault balance should should be same as before') + } else { + const recoverReceipt = await recoverAction() + assert.equal((await token.balanceOf(target.address)).valueOf(), 0, 'Target balance should be 0') + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance.plus(initialBalance).plus(amount), 'Vault balance should include recovered amount') + + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'vault'), vault.address, 'RecoverToVault event should have correct vault') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'token'), token.address, 'RecoverToVault event should have correct token') + assert.equal(getEvent(recoverReceipt, 'RecoverToVault', 'amount'), amount, 'RecoverToVault event should have correct amount') + assertEvent(recoverReceipt, 'RecoverToVault', 1) + } + } + + const failingRecoverTokens = async ({ tokenContract, target, vault }) => { + const amount = 1 + const token = await tokenContract.new(accounts[0], 1000) + const initialBalance = await token.balanceOf(target.address) + const initialVaultBalance = await token.balanceOf(vault.address) + await token.transfer(target.address, amount) + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') + + // Stop token from being transferable + await token.setAllowTransfer(false) + + // Try to transfer + await assertRevert(() => target.transferToVault(token.address)) + + assert.equal((await token.balanceOf(target.address)).valueOf(), initialBalance.plus(amount), 'Target balance should be same as before') + assert.equal((await token.balanceOf(vault.address)).valueOf(), initialVaultBalance, 'Vault balance should should be same as before') } const failWithoutVault = async (target, kernel) => { @@ -58,12 +107,28 @@ contract('Proxy funds', accounts => { const initialBalance = await getBalance(target.address) await kernel.setRecoveryVaultAppId(vaultId) const r = await target.sendTransaction({ value: 1, gas: SEND_ETH_GAS }) - assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount)) + assert.equal((await getBalance(target.address)).valueOf(), initialBalance.plus(amount), 'Target initial balance should be correct') return assertRevert(async () => { await target.transferToVault(ETH) }) } + // Token test groups + const tokenTestGroups = [ + { + title: 'standards compliant, reverting token', + tokenContract: TokenMock, + }, + { + title: 'standards compliant, non-reverting token', + tokenContract: TokenReturnFalseMock, + }, + { + title: 'non-standards compliant, missing return token', + tokenContract: TokenReturnMissingMock, + }, + ] + // Initial setup before(async () => { aclBase = await ACL.new() @@ -141,9 +206,25 @@ contract('Proxy funds', accounts => { ) ) - it('kernel recovers tokens', async () => { - await recoverTokens(kernel, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`kernel recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + vault, + target: kernel + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`kernel reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + vault, + target: kernel, + }) + }) + } context('> App without kernel', () => { beforeEach(async () => { @@ -152,15 +233,16 @@ contract('Proxy funds', accounts => { }) it('does not recover ETH', skipCoverageIfVaultProxy(async () => - await assertRevert( - () => recoverEth(target, vault) - ) + await recoverEth({ target, vault, shouldFail: true }) )) it('does not recover tokens', async () => - await assertRevert( - () => recoverTokens(target, vault) - ) + await recoverTokens({ + target, + vault, + shouldFail: true, + tokenContract: TokenMock, + }) ) }) @@ -187,13 +269,29 @@ contract('Proxy funds', accounts => { }) }) - it('recovers ETH', skipCoverageIfVaultProxy( - async () => await recoverEth(target, vault) + it('recovers ETH', skipCoverageIfVaultProxy(async () => + await recoverEth({ target, vault }) )) - it('recovers tokens', async () => { - await recoverTokens(target, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + target, + vault, + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + target, + vault, + }) + }) + } it('fails if vault is not contract', async () => { await failWithoutVault(target, kernel) @@ -211,16 +309,30 @@ contract('Proxy funds', accounts => { target = app }) - it('does not allow recovering ETH', skipCoverageIfVaultProxy( + it('does not allow recovering ETH', skipCoverageIfVaultProxy(async () => // Conditional stub doesnt allow eth recoveries - () => assertRevert( - () => recoverEth(target, vault) - ) + await recoverEth({ target, vault, shouldFail: true }) )) - it('allows recovering tokens', async () => { - await recoverTokens(target, vault) - }) + for ({ title, tokenContract} of tokenTestGroups) { + it(`allows recovers ${title}`, async () => { + await recoverTokens({ + tokenContract, + target, + vault, + }) + }) + } + + for ({ title, tokenContract} of tokenTestGroups) { + it(`reverts on failing recovery for ${title}`, async () => { + await failingRecoverTokens({ + tokenContract, + target, + vault, + }) + }) + } }) }) } @@ -255,7 +367,7 @@ contract('Proxy funds', accounts => { }) it('recovers ETH from the kernel', skipCoverage(async () => { - await recoverEth(kernel, vault) + await recoverEth({ target: kernel, vault }) })) }) }) diff --git a/test/safe_erc20.js b/test/safe_erc20.js new file mode 100644 index 000000000..7996f091e --- /dev/null +++ b/test/safe_erc20.js @@ -0,0 +1,130 @@ +const { assertRevert } = require('./helpers/assertThrow') + +// Mocks +const SafeERC20Mock = artifacts.require('SafeERC20Mock') +const TokenMock = artifacts.require('TokenMock') +const TokenReturnFalseMock = artifacts.require('TokenReturnFalseMock') +const TokenReturnMissingMock = artifacts.require('TokenReturnMissingMock') + +const assertMockResult = (receipt, result) => { + const events = receipt.logs.filter(x => x.event == 'Result') + const eventArg = events[0].args.result + assert.equal(eventArg, result, `Result not expected (got ${eventArg} instead of ${result})`) +} + +contract('SafeERC20', accounts => { + const [owner, receiver] = accounts + const initialBalance = 10000 + let safeERC20Mock + + before(async () => { + safeERC20Mock = await SafeERC20Mock.new() + }) + + const testGroups = [ + { + title: 'Standards compliant, reverting token', + tokenContract: TokenMock, + }, + { + title: 'Standards compliant, non-reverting token', + tokenContract: TokenReturnFalseMock, + }, + { + title: 'Non-standards compliant, missing return token', + tokenContract: TokenReturnMissingMock, + }, + ] + + for ({ title, tokenContract} of testGroups) { + context(`> ${title}`, () => { + let tokenMock + + beforeEach(async () => { + tokenMock = await tokenContract.new(owner, initialBalance) + }) + + it('can correctly transfer', async () => { + // Add balance to the mock + const transferAmount = 5000 + await tokenMock.transfer(safeERC20Mock.address, transferAmount) + + // Transfer it all back from the mock + const receipt = await safeERC20Mock.transfer(tokenMock.address, owner, transferAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should be correct') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should be correct') + }) + + it('detects failed transfer', async () => { + // Attempt transfer when mock has no balance + const receipt = await safeERC20Mock.transfer(tokenMock.address, owner, 1000) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should stay the same') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('can correctly approve', async () => { + const approvedAmount = 5000 + + // Create approval from the mock + const receipt = await safeERC20Mock.approve(tokenMock.address, receiver, approvedAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.allowance(safeERC20Mock.address, receiver)).valueOf(), approvedAmount, 'Allowance of receiver should be correct') + }) + + it('detects failed approve', async () => { + const preApprovedAmount = 5000 + + // Create pre-exisiting approval + await safeERC20Mock.approve(tokenMock.address, receiver, preApprovedAmount) + + // Attempt to create another approval without reseting it back to 0 + const receipt = await safeERC20Mock.approve(tokenMock.address, receiver, preApprovedAmount - 500) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.allowance(safeERC20Mock.address, receiver)).valueOf(), preApprovedAmount, 'Allowance of receiver should be the pre-existing value') + }) + + it('can correctly transferFrom', async () => { + // Create approval + const approvedAmount = 5000 + await tokenMock.approve(safeERC20Mock.address, approvedAmount) + + // Transfer to receiver through the mock + const receipt = await safeERC20Mock.transferFrom(tokenMock.address, owner, receiver, approvedAmount) + assertMockResult(receipt, true) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance - approvedAmount, 'Balance of owner should be correct') + assert.equal((await tokenMock.balanceOf(receiver)).valueOf(), approvedAmount, 'Balance of receiver should be correct') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('detects failed transferFrom', async () => { + // Attempt transfer to receiver through the mock when mock wasn't approved + const receipt = await safeERC20Mock.transferFrom(tokenMock.address, owner, receiver, 5000) + + assertMockResult(receipt, false) + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), initialBalance, 'Balance of owner should stay the same') + assert.equal((await tokenMock.balanceOf(receiver)).valueOf(), 0, 'Balance of receiver should stay the same') + assert.equal((await tokenMock.balanceOf(safeERC20Mock.address)).valueOf(), 0, 'Balance of mock should stay the same') + }) + + it('gives correct value with static allowance', async () => { + // Create approval + const approvedAmount = 5000 + await tokenMock.approve(safeERC20Mock.address, approvedAmount) + + const approval = (await safeERC20Mock.allowance(tokenMock.address, owner, safeERC20Mock.address)).valueOf() + assert.equal(approval, approvedAmount, 'Mock should return correct allowance') + assert.equal((await tokenMock.allowance(owner, safeERC20Mock.address)).valueOf(), approval, "Mock should match token contract's allowance") + }) + + it('gives correct value with static balanceOf', async () => { + const balance = (await safeERC20Mock.balanceOf(tokenMock.address, owner)).valueOf() + assert.equal(balance, initialBalance, 'Mock should return correct balance') + assert.equal((await tokenMock.balanceOf(owner)).valueOf(), balance, "Mock should match token contract's balance") + }) + }) + } +}) diff --git a/test/time_helpers.js b/test/time_helpers.js index 828640260..599afe88b 100644 --- a/test/time_helpers.js +++ b/test/time_helpers.js @@ -1,4 +1,4 @@ -contract('TimeHelpers test', accounts => { +contract('TimeHelpers', accounts => { let timeHelpersMock before(async () => {