Feature #909 proxy delegatecall (#1152)

packages/protocol/contracts/common/MultiSig.sol

@@ -2,6 +2,7 @@ pragma solidity ^0.5.3;
 /* solhint-disable no-inline-assembly, avoid-low-level-calls, func-name-mixedcase, func-order */
 
 import "./Initializable.sol";
+import "./libraries/AddressesHelper.sol";
 
 
 /// @title Multisignature wallet - Allows multiple parties to agree on transactions before
@@ -261,6 +262,10 @@ contract MultiSig is Initializable {
     returns (bool)
   {
     bool result;
+
+    if (dataLength > 0)
+      require(AddressesHelper.isContract(destination), "Invalid contract address");
+
     /* solhint-disable max-line-length */
     assembly {
       let x := mload(0x40)   // "Allocate" memory for output (0x40 is where "free memory" pointer is stored by convention)

packages/protocol/contracts/common/Proxy.sol

@@ -1,6 +1,7 @@
 pragma solidity ^0.5.3;
 /* solhint-disable no-inline-assembly, no-complex-fallback, avoid-low-level-calls */
 
+import "./libraries/AddressesHelper.sol";
 
 /**
  * @title A Proxy utilizing the Unstructured Storage pattern.
@@ -32,8 +33,15 @@ contract Proxy {
   function () external payable {
     bytes32 implementationPosition = IMPLEMENTATION_POSITION;
 
+    address implementationAddress;
+
+    assembly {
+      implementationAddress := sload(implementationPosition)
+    }
+
+    require(AddressesHelper.isContract(implementationAddress), "Invalid contract address");
+
     assembly {
-      let implementationAddress := sload(implementationPosition)
       let newCallDataPosition := mload(0x40)
       mstore(0x40, add(newCallDataPosition, calldatasize))
 
@@ -114,6 +122,8 @@ contract Proxy {
   function _setImplementation(address implementation) public onlyOwner {
     bytes32 implementationPosition = IMPLEMENTATION_POSITION;
 
+    require(AddressesHelper.isContract(implementation), "Invalid contract address");
+
     assembly {
       sstore(implementationPosition, implementation)
     }

packages/protocol/contracts/common/libraries/AddressesHelper.sol

@@ -0,0 +1,21 @@
+pragma solidity ^0.5.3;
+
+/**
+ * @title Library with support functions to deal with addresses
+ */
+library AddressesHelper {
+
+  /**
+   * @dev isContract detect whether the address is 
+   *      a contract address or externally owned account (EOA)
+   * WARNING: Calling this function from a constructor will return false
+   *          independently if the address given as parameter is a contract or EOA
+   * @return true if it is a contract address
+   */
+  function isContract(address addr) internal view returns (bool) {
+    uint256 size;
+    /* solium-disable-next-line security/no-inline-assembly */
+    assembly { size := extcodesize(addr) }
+    return size > 0;
+  }
+}

packages/protocol/contracts/governance/Proposals.sol

@@ -4,6 +4,7 @@ import "openzeppelin-solidity/contracts/math/SafeMath.sol";
 import "solidity-bytes-utils/contracts/BytesLib.sol";
 
 import "../common/FixidityLib.sol";
+import "../common/libraries/AddressesHelper.sol";
 
 /**
  * @title A library operating on Celo Governance proposals.
@@ -324,6 +325,10 @@ library Proposals {
     returns (bool)
   {
     bool result;
+
+    if (dataLength > 0)
+      require(AddressesHelper.isContract(destination), "Invalid contract address");
+
     /* solhint-disable no-inline-assembly */
     assembly {
       /* solhint-disable max-line-length */

packages/protocol/test/common/proxy.ts

@@ -116,6 +116,13 @@ contract('Proxy', (accounts: string[]) => {
       assert.equal(events[0].event, 'ImplementationSet')
     })
 
+    it('should not allow to call a non contract address', async () =>
+      assertRevert(
+        proxy._setAndInitializeImplementation(accounts[1], initializeData(42), {
+          from: accounts[1],
+        })
+      ))
+
     it('should not allow a non-owner to set an implementation', async () =>
       assertRevert(
         proxy._setAndInitializeImplementation(hasInitializer.address, initializeData(42), {

packages/protocol/test/governance/governance.ts

@@ -1625,6 +1625,29 @@ contract('Governance', (accounts: string[]) => {
           await assertRevert(governance.execute(proposalId, index))
         })
       })
+
+      describe('when the proposal cannot execute because it is not a contract address', () => {
+        beforeEach(async () => {
+          await governance.propose(
+            [transactionSuccess1.value],
+            [accounts[1]],
+            transactionSuccess1.data,
+            [transactionSuccess1.data.length],
+            // @ts-ignore: TODO(mcortesi) fix typings for TransactionDetails
+            { value: minDeposit }
+          )
+          await timeTravel(dequeueFrequency, web3)
+          await governance.approve(proposalId, index)
+          await timeTravel(approvalStageDuration, web3)
+          await mockLockedGold.setWeight(account, weight)
+          await governance.vote(proposalId, index, value)
+          await timeTravel(referendumStageDuration, web3)
+        })
+
+        it('should revert', async () => {
+          await assertRevert(governance.execute(proposalId, index))
+        })
+      })
     })
 
     describe('when executing a proposal with two transactions', () => {