AppProxyPinned: check that pinned code is a contract

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()));
     }
 
     /**

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/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/KernelSetAppMock.sol

@@ -0,0 +1,13 @@
+pragma solidity 0.4.18;
+
+import "../../contracts/kernel/Kernel.sol";
+
+contract KernelSetAppMock is Kernel {
+    function KernelSetAppMock() Kernel(false) public {
+    }
+
+    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 () => {