Proxies: add checks for base being a contract (#394)

contracts/apps/AppProxyPinned.sol

@@ -1,10 +1,11 @@
 pragma solidity 0.4.18;
 
 import "../common/UnstructuredStorage.sol";
+import "../common/IsContract.sol";
 import "./AppProxyBase.sol";
 
 
-contract AppProxyPinned is AppProxyBase {
+contract AppProxyPinned is IsContract, AppProxyBase {
     using UnstructuredStorage for bytes32;
 
     // keccak256("aragonOS.appStorage.pinnedCode"), used by Proxy Pinned
@@ -21,7 +22,7 @@ contract AppProxyPinned is AppProxyBase {
         public // solium-disable-line visibility-first
     {
         setPinnedCode(getAppBase(_appId));
-        require(pinnedCode() != address(0));
+        require(isContract(pinnedCode()));
     }
 
     /**

contracts/common/Initializable.sol

@@ -5,7 +5,6 @@
 pragma solidity ^0.4.18;
 
 import "./UnstructuredStorage.sol";
-import "../apps/AppStorage.sol";
 import "../common/TimeHelpers.sol";
 
 

contracts/kernel/KernelProxy.sol

@@ -3,15 +3,17 @@ pragma solidity 0.4.18;
 import "./IKernel.sol";
 import "./KernelStorage.sol";
 import "../common/DepositableDelegateProxy.sol";
+import "../common/IsContract.sol";
 
 
-contract KernelProxy is KernelStorage, DepositableDelegateProxy {
+contract KernelProxy is KernelStorage, IsContract, DepositableDelegateProxy {
     /**
     * @dev KernelProxy is a proxy contract to a kernel implementation. The implementation
     *      can update the reference, which effectively upgrades the contract
     * @param _kernelImpl Address of the contract used as implementation for kernel
     */
     function KernelProxy(IKernel _kernelImpl) public {
+        require(isContract(address(_kernelImpl)));
         apps[KERNEL_APP] = _kernelImpl;
     }
 
@@ -28,5 +30,4 @@ contract KernelProxy is KernelStorage, DepositableDelegateProxy {
     function implementation() public view returns (address) {
         return apps[KERNEL_APP];
     }
-
 }

test/app_proxy.js

@@ -13,6 +13,7 @@ const AppProxyPinned = artifacts.require('AppProxyPinned')
 const AppStub = artifacts.require('AppStub')
 const AppStub2 = artifacts.require('AppStub2')
 const ERCProxyMock = artifacts.require('ERCProxyMock')
+const KernelSetAppMock = artifacts.require('KernelSetAppMock')
 
 const APP_ID = hash('stub.aragonpm.test')
 const ZERO_ADDR = '0x0000000000000000000000000000000000000000'
@@ -109,19 +110,25 @@ contract('App proxy', accounts => {
       })
 
       onlyAppProxyPinned(() => {
+        const FAKE_APP_ID = hash('fake.aragonpm.test')
+
         it("fails if code hasn't been set yet", async () => {
-          const fakeAppId = hash('fake.aragonpm.test')
-          return assertRevert(async () => {
-            await AppProxyPinned.new(kernel.address, fakeAppId, EMPTY_BYTES)
+          await assertRevert(async () => {
+            await AppProxyPinned.new(kernel.address, FAKE_APP_ID, EMPTY_BYTES)
           })
         })
-      })
 
-      context('> Fails on bad kernel', () => {
-        beforeEach(async () => {
-          await kernel.setApp(APP_BASES_NAMESPACE, APP_ID, appBase1.address)
+        it("fails if code set isn't a contract", async () => {
+          const kernelMock = await KernelSetAppMock.new()
+          await kernelMock.setApp(APP_BASES_NAMESPACE, FAKE_APP_ID, '0x1234')
+
+          await assertRevert(async () => {
+            await AppProxyPinned.new(kernelMock.address, FAKE_APP_ID, EMPTY_BYTES)
+          })
         })
+      })
 
+      context('> Fails on bad kernel', () => {
         it('fails if kernel address is 0', async () => {
           return assertRevert(async () => {
             await appProxyContract.new(ZERO_ADDR, APP_ID, EMPTY_BYTES)

test/keccak_constants.js

@@ -93,7 +93,10 @@ contract('Constants', accounts => {
   })
 
   it('checks AppProxyPinned unstructured storage constants', async () => {
-    const app = await getContract('AppStubPinnedStorage').new()
+    // Set up AppStubPinnedStorage
+    const fakeApp = await getContract('AppStub').new()
+    const kernelMock = await getContract('KernelPinnedStorageMock').new(fakeApp.address)
+    const app = await getContract('AppStubPinnedStorage').new(kernelMock.address)
 
     assert.equal(await app.getPinnedCodePosition(), await keccakConstants.pinnedCodePosition(), "pinnedCodePosition doesn't match")
   })

test/kernel_apps.js

@@ -14,7 +14,7 @@ const AppProxyPinned = artifacts.require('AppProxyPinned')
 const AppStub = artifacts.require('AppStub')
 const AppStub2 = artifacts.require('AppStub2')
 const ERCProxyMock = artifacts.require('ERCProxyMock')
-const KernelMock = artifacts.require('KernelMock')
+const KernelOverloadMock = artifacts.require('KernelOverloadMock')
 
 const APP_ID = hash('stub.aragonpm.test')
 const ZERO_ADDR = '0x0000000000000000000000000000000000000000'
@@ -145,8 +145,8 @@ contract('Kernel apps', accounts => {
                     it("also sets the default app when using the overloaded version", async () => {
                         let appProxyAddr
 
-                        // Create KernelMock instance so we can use the overloaded version
-                        const kernelMock = await KernelMock.new(kernel.address)
+                        // Create KernelOverloadMock instance so we can use the overloaded version
+                        const kernelMock = await KernelOverloadMock.new(kernel.address)
 
                         await withAppManagerPermission(kernelMock.address, async () => {
                             const receipt = await kernelMock[newInstanceFn](APP_ID, appBase1.address, true)

test/kernel_upgrade.js

@@ -5,6 +5,8 @@ const Kernel = artifacts.require('Kernel')
 const KernelProxy = artifacts.require('KernelProxy')
 const UpgradedKernel = artifacts.require('UpgradedKernel')
 
+const ZERO_ADDR = '0x0000000000000000000000000000000000000000'
+
 // Only applicable to KernelProxy instances
 contract('Kernel upgrade', accounts => {
     let aclBase, kernelBase, upgradedBase, kernelAddr, kernel, acl
@@ -39,6 +41,18 @@ contract('Kernel upgrade', accounts => {
         assert.equal(proxyType, (await kernelProxy.UPGRADEABLE()).toString(), "Proxy type should be 2 (upgradeable)")
     })
 
+    it('fails to create a KernelProxy if the base is 0', async () => {
+        return assertRevert(async () => {
+            await KernelProxy.new(ZERO_ADDR)
+        })
+    })
+
+    it('fails to create a KernelProxy if the base is not a contract', async () => {
+        return assertRevert(async () => {
+            await KernelProxy.new('0x1234')
+        })
+    })
+
     it('fails to upgrade kernel without permission', async () => {
         return assertRevert(async () => {
             await kernel.setApp(CORE_NAMESPACE, KERNEL_APP_ID, accounts[0])

test/mocks/AppStubPinnedStorage.sol

@@ -2,21 +2,29 @@ pragma solidity 0.4.18;
 
 import "../../contracts/apps/AppProxyPinned.sol";
 import "../../contracts/kernel/IKernel.sol";
+import "../../contracts/kernel/Kernel.sol";
 
 
-contract AppStubPinnedStorage is AppProxyPinned {
+contract FakeAppConstants {
     bytes32 constant FAKE_APP_ID = keccak256('FAKE_APP_ID');
-    address constant FAKE_APP_ADDR = address(1);
+}
+
+contract KernelPinnedStorageMock is Kernel, FakeAppConstants {
+    bytes32 constant FAKE_APP_ID = keccak256('FAKE_APP_ID');
+    function KernelPinnedStorageMock(address _fakeApp) Kernel(false) public {
+        _setApp(APP_BASES_NAMESPACE, FAKE_APP_ID, _fakeApp);
+    }
+}
 
-    // Allow the mock to be created without any arguments
-    function AppStubPinnedStorage()
-        AppProxyPinned(IKernel(0), FAKE_APP_ID, new bytes(0))
-        public // solium-disable-line visibility-first
-    {}
 
-    // Overload base to return our own fake address
-    function getAppBase(bytes32 _appId) internal view returns (address) {
-        return FAKE_APP_ADDR;
+// Testing this contract is a bit of a pain... we can't overload anything to make the contract check
+// pass in the constructor, so we're forced to initialize this with a mocked Kernel that already
+// sets a contract for the fake app.
+contract AppStubPinnedStorage is AppProxyPinned, FakeAppConstants {
+    function AppStubPinnedStorage(KernelPinnedStorageMock _mockKernel)
+        AppProxyPinned(IKernel(_mockKernel), FAKE_APP_ID, new bytes(0))
+        public // solium-disable-line visibility-first
+    {
     }
 
     function setPinnedCodeExt(address _pinnedCode) public {

test/mocks/KernelMock.sol → test/mocks/KernelOverloadMock.sol

@@ -8,12 +8,12 @@ import "../../contracts/lib/misc/ERCProxy.sol";
  * https://github.com/trufflesuite/truffle/issues/569
  * https://github.com/trufflesuite/truffle/issues/737
  */
-contract KernelMock {
+contract KernelOverloadMock {
     Kernel kernel;
 
     event NewAppProxy(address proxy);
 
-    function KernelMock(address _kernel) public {
+    function KernelOverloadMock(address _kernel) public {
         kernel = Kernel(_kernel);
     }
 

test/mocks/KernelSetAppMock.sol

@@ -0,0 +1,14 @@
+pragma solidity 0.4.18;
+
+import "../../contracts/kernel/Kernel.sol";
+
+contract KernelSetAppMock is Kernel {
+    function KernelSetAppMock() Kernel(false) public {
+    }
+
+    // Overloaded mock to bypass the auth and isContract checks
+    function setApp(bytes32 _namespace, bytes32 _name, address _app) public returns (bytes32 id) {
+        id = keccak256(_namespace, _name);
+        apps[id] = _app;
+    }
+}

test/unstructured_storage.js

@@ -1,14 +1,22 @@
+const AppStub = artifacts.require('AppStub')
 const AppStubStorage = artifacts.require('AppStubStorage')
 const AppStubPinnedStorage = artifacts.require('AppStubPinnedStorage')
 const Kernel = artifacts.require('Kernel')
 
+// Mocks
+const KernelPinnedStorageMock = artifacts.require('KernelPinnedStorageMock')
+
 contract('Unstructured storage', accounts => {
   let app, kernel
 
   beforeEach(async () => {
     app = await AppStubStorage.new()
-    appPinned = await AppStubPinnedStorage.new()
     kernel = await Kernel.new(true)
+
+    // Set up AppStubPinnedStorage
+    const fakeApp = await AppStub.new()
+    const kernelMock = await KernelPinnedStorageMock.new(fakeApp.address)
+    appPinned = await AppStubPinnedStorage.new(kernelMock.address)
   })
 
   it('tests init block', async () => {