Update for Istanbul context

Pulls in changes from: * fb93dee (Istanbul) * 2bff3be (Istanbul) Also pulls in cosmetic / test updates from: * 48d34de (coverage) * 635dd96 (test) * a59fec6 (test) * ad46b86 (cosmetic) * 243d330 (coverage) Co-Authored-By: Brett Sun <[email protected]>
aragon · Feb 6, 2020 · 5752a41 · 5752a41
1 parent 96d9940
commit 5752a41
Show file tree

Hide file tree

Showing 27 changed files with 719 additions and 372 deletions.
diff --git a/contracts/common/DepositableDelegateProxy.sol b/contracts/common/DepositableDelegateProxy.sol
@@ -8,14 +8,37 @@ contract DepositableDelegateProxy is DepositableStorage, DelegateProxy {
     event ProxyDeposit(address sender, uint256 value);
 
     function () external payable {
-        // send / transfer
-        if (gasleft() < FWD_GAS_LIMIT) {
-            require(msg.value > 0 && msg.data.length == 0);
-            require(isDepositable());
-            emit ProxyDeposit(msg.sender, msg.value);
-        } else { // all calls except for send or transfer
-            address target = implementation();
-            delegatedFwd(target, msg.data);
+        uint256 forwardGasThreshold = FWD_GAS_LIMIT;
+        bytes32 isDepositablePosition = DEPOSITABLE_POSITION;
+
+        // Optimized assembly implementation to prevent EIP-1884 from breaking deposits, reference code in Solidity:
+        // https://github.com/aragon/aragonOS/blob/v4.2.1/contracts/common/DepositableDelegateProxy.sol#L10-L20
+        assembly {
+            // Continue only if the gas left is lower than the threshold for forwarding to the implementation code,
+            // otherwise continue outside of the assembly block.
+            if lt(gas, forwardGasThreshold) {
+                // Only accept the deposit and emit an event if all of the following are true:
+                // the proxy accepts deposits (isDepositable), msg.data.length == 0, and msg.value > 0
+                if and(and(sload(isDepositablePosition), iszero(calldatasize)), gt(callvalue, 0)) {
+                    // Equivalent Solidity code for emitting the event:
+                    // emit ProxyDeposit(msg.sender, msg.value);
+
+                    let logData := mload(0x40) // free memory pointer
+                    mstore(logData, caller) // add 'msg.sender' to the log data (first event param)
+                    mstore(add(logData, 0x20), callvalue) // add 'msg.value' to the log data (second event param)
+
+                    // Emit an event with one topic to identify the event: keccak256('ProxyDeposit(address,uint256)') = 0x15ee...dee1
+                    log1(logData, 0x40, 0x15eeaa57c7bd188c1388020bcadc2c436ec60d647d36ef5b9eb3c742217ddee1)
+
+                    stop() // Stop. Exits execution context
+                }
+
+                // If any of above checks failed, revert the execution (if ETH was sent, it is returned to the sender)
+                revert(0, 0)
+            }
         }
+
+        address target = implementation();
+        delegatedFwd(target, msg.data);
     }
 }
diff --git a/contracts/lib/ens/AbstractENS.sol b/contracts/lib/ens/AbstractENS.sol
@@ -1,3 +1,5 @@
+// See https://github.com/ensdomains/ens/blob/7e377df83f/contracts/AbstractENS.sol
+
 pragma solidity ^0.4.15;
 
 

diff --git a/contracts/lib/ens/ENS.sol b/contracts/lib/ens/ENS.sol
@@ -1,7 +1,9 @@
+// See https://github.com/ensdomains/ens/blob/7e377df83f/contracts/ENS.sol
+
 pragma solidity ^0.4.0;
 
 
-import './AbstractENS.sol';
+import "./AbstractENS.sol";
 
 /**
  * The ENS registry contract.

diff --git a/contracts/lib/ens/PublicResolver.sol b/contracts/lib/ens/PublicResolver.sol
@@ -1,6 +1,8 @@
+// See https://github.com/ensdomains/ens/blob/7e377df83f/contracts/PublicResolver.sol
+
 pragma solidity ^0.4.0;
 
-import './AbstractENS.sol';
+import "./AbstractENS.sol";
 
 /**
  * A simple resolver anyone can use; only allows the owner of a node to set its

diff --git a/contracts/test/helpers/EthSender.sol b/contracts/test/helpers/EthSender.sol
@@ -0,0 +1,8 @@
+pragma solidity 0.4.24;
+
+
+contract EthSender {
+    function sendEth(address to) external payable {
+        to.transfer(msg.value);
+    }
+}
diff --git a/contracts/test/helpers/ProxyTarget.sol b/contracts/test/helpers/ProxyTarget.sol
@@ -0,0 +1,17 @@
+pragma solidity 0.4.24;
+
+contract ProxyTargetWithoutFallback {
+    event Pong();
+
+    function ping() external {
+      emit Pong();
+    }
+}
+
+contract ProxyTargetWithFallback is ProxyTargetWithoutFallback {
+    event ReceivedEth();
+
+    function () external payable {
+      emit ReceivedEth();
+    }
+}
diff --git 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/DepositableDelegateProxyMock.sol b/contracts/test/mocks/DepositableDelegateProxyMock.sol
@@ -0,0 +1,24 @@
+pragma solidity 0.4.24;
+
+import "../../common/DepositableDelegateProxy.sol";
+
+
+contract DepositableDelegateProxyMock is DepositableDelegateProxy {
+    address private implementationMock;
+
+    function enableDepositsOnMock() external {
+        setDepositable(true);
+    }
+
+    function setImplementationOnMock(address _implementationMock) external {
+        implementationMock = _implementationMock;
+    }
+
+    function implementation() public view returns (address) {
+        return implementationMock;
+    }
+
+    function proxyType() public pure returns (uint256 proxyTypeId) {
+        return UPGRADEABLE;
+    }
+}
diff --git a/package.json b/package.json
@@ -39,12 +39,13 @@
     "ethereumjs-abi": "^0.6.5",
     "ganache-cli": "^6.1.0",
     "mocha-lcov-reporter": "^1.3.0",
-    "solidity-coverage": "0.5.8",
-    "solium": "^1.1.8",
+    "solidity-coverage": "0.6.2",
+    "solium": "^1.2.3",
     "truffle": "4.1.14",
     "truffle-bytecode-manager": "^1.1.1",
     "truffle-extract": "^1.2.1",
-    "web3-utils": "1.0.0-beta.33"
+    "web3-eth-abi": "1.2.5",
+    "web3-utils": "1.2.5"
   },
   "dependencies": {
     "mkdirp": "^0.5.1",

diff --git a/test/apm_registry.js b/test/apm_registry.js
@@ -19,6 +19,9 @@ const APMRegistryFactoryMock = artifacts.require('APMRegistryFactoryMock')
 
 const getRepoFromLog = receipt => receipt.logs.filter(x => x.event == 'NewRepo')[0].args.repo
 
+const EMPTY_BYTES = '0x'
+const ZERO_ADDR = '0x0000000000000000000000000000000000000000'
+
 contract('APMRegistry', accounts => {
     let baseDeployed, baseAddrs, ensFactory, apmFactory, daoFactory, ens, registry, acl
     const ensOwner = accounts[0]
@@ -38,11 +41,11 @@ contract('APMRegistry', accounts => {
 
         const kernelBase = await Kernel.new(true) // petrify immediately
         const aclBase = await ACL.new()
-        daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, '0x00')
+        daoFactory = await DAOFactory.new(kernelBase.address, aclBase.address, ZERO_ADDR)
     })
 
     beforeEach(async () => {
-        apmFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, '0x0', ensFactory.address)
+        apmFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ZERO_ADDR, ensFactory.address)
         ens = ENS.at(await apmFactory.ens())
 
         const receipt = await apmFactory.newAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner)
@@ -60,7 +63,7 @@ contract('APMRegistry', accounts => {
     it('inits with existing ENS deployment', async () => {
         const receipt = await ensFactory.newENS(accounts[0])
         const ens2 = ENS.at(receipt.logs.filter(l => l.event == 'DeployENS')[0].args.ens)
-        const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, '0x00')
+        const newFactory = await APMRegistryFactory.new(daoFactory.address, ...baseAddrs, ens2.address, ZERO_ADDR)
 
         await ens2.setSubnodeOwner(namehash('eth'), '0x'+keccak256('aragonpm'), newFactory.address)
         const receipt2 = await newFactory.newAPM(namehash('eth'), '0x'+keccak256('aragonpm'), apmOwner)
@@ -81,16 +84,27 @@ contract('APMRegistry', accounts => {
     })
 
     it('can create repo with version and dev can create new versions', async () => {
-        const receipt = await registry.newRepoWithVersion('test', repoDev, [1, 0, 0], '0x00', '0x00', { from: apmOwner })
+        const receipt = await registry.newRepoWithVersion('test', repoDev, [1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: apmOwner })
         const repo = Repo.at(getRepoFromLog(receipt))
 
         assert.equal(await repo.getVersionsCount(), 1, 'should have created version')
 
-        await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: repoDev })
+        await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev })
 
         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, ZERO_ADDR)
+
+        // 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 +117,7 @@ contract('APMRegistry', accounts => {
         })
     })
 
-    context('creating test.aragonpm.eth repo', () => {
+    context('> Creating test.aragonpm.eth repo', () => {
         let repo = {}
 
         beforeEach(async () => {
@@ -130,56 +144,56 @@ contract('APMRegistry', accounts => {
         })
 
         it('repo dev can create versions', async () => {
-            await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: repoDev })
-            await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: repoDev })
+            await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev })
+            await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev })
 
             assert.equal(await repo.getVersionsCount(), 2, 'should have created versions')
         })
 
         it('repo dev can authorize someone to interact with repo', async () => {
-            await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: repoDev })
+            await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev })
             const newOwner = accounts[8]
 
             await acl.grantPermission(newOwner, repo.address, await repo.CREATE_VERSION_ROLE(), { from: repoDev })
 
-            await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: newOwner })
-            await repo.newVersion([2, 1, 0], '0x00', '0x00', { from: repoDev }) // repoDev can still create them
+            await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: newOwner })
+            await repo.newVersion([2, 1, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev }) // repoDev can still create them
 
             assert.equal(await repo.getVersionsCount(), 3, 'should have created versions')
         })
 
         it('repo dev can no longer create versions if permission is removed', async () => {
-            await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: repoDev })
+            await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev })
             await acl.revokePermission(repoDev, repo.address, await repo.CREATE_VERSION_ROLE(), { from: repoDev })
 
             return assertRevert(async () => {
-                await repo.newVersion([2, 0, 0], '0x00', '0x00', { from: repoDev })
+                await repo.newVersion([2, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: repoDev })
             })
         })
 
         it('cannot create versions if not in ACL', async () => {
             return assertRevert(async () => {
-                await repo.newVersion([1, 0, 0], '0x00', '0x00', { from: notOwner })
+                await repo.newVersion([1, 0, 0], ZERO_ADDR, EMPTY_BYTES, { from: notOwner })
             })
         })
     })
 
-    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)
             })
         })
     })