SB script dry execution (#237)

* code formatting * dryExecute * dryExecute to return delegatecall revert reason * dryExecute: return void, copy only on revert, comments * dryExecute: minor code simplification * dryExecute: extra tests with no reason&out of gas
Augmint · Jun 7, 2019 · 522eb25 · 522eb25
1 parent 1a847c8
commit 522eb25
Show file tree

Hide file tree

Showing 3 changed files with 144 additions and 28 deletions.
diff --git a/contracts/generic/MultiSig.sol b/contracts/generic/MultiSig.sol
@@ -84,16 +84,23 @@ contract MultiSig {
         Script storage script = scripts[scriptAddress];
         require(script.state == ScriptState.Approved, "script state must be Approved");
 
-        // passing scriptAddress to allow called script access its own public fx-s if needed
-        if (scriptAddress.delegatecall.gas(gasleft() - 23000)
-            (abi.encodeWithSignature("execute(address)", scriptAddress))) {
-            script.state = ScriptState.Done;
-            result = true;
-        } else {
-            script.state = ScriptState.Failed;
-            result = false;
+        result = _execute(scriptAddress);
+    }
+    /* Function to test scripts without signatures - always reverts.
+        It's even possible to test scripts without deployment if script deployed and dry run on a ganache */
+    function dryExecute(address scriptAddress) public {
+        bool result = _execute(scriptAddress);
+
+        if(!result) { // if delegatecall failed then revert with the revert returndata
+            // solium-disable-next-line security/no-inline-assembly
+            assembly {
+                let ptr := mload(0x40) // starts from the "free memory pointer"
+                returndatacopy(ptr, 0, returndatasize) // retreive delegatecall revert reason message
+                revert(ptr, returndatasize)  // revert with the reason message from delegeatecall
+            }
         }
-        emit ScriptExecuted(scriptAddress, result);
+
+        revert("dryExecute success");
     }
 
     function cancelScript(address scriptAddress) public {
@@ -110,7 +117,7 @@ contract MultiSig {
     /* requires quorum so it's callable only via a script executed by this contract */
     function addSigners(address[] signers) public {
         require(msg.sender == address(this), "only callable via MultiSig");
-        for (uint i= 0; i < signers.length; i++) {
+        for (uint i = 0; i < signers.length; i++) {
             if (!isSigner[signers[i]]) {
                 require(signers[i] != address(0), "new signer must not be 0x0");
                 activeSignersCount++;
@@ -124,7 +131,7 @@ contract MultiSig {
     /* requires quorum so it's callable only via a script executed by this contract */
     function removeSigners(address[] signers) public {
         require(msg.sender == address(this), "only callable via MultiSig");
-        for (uint i= 0; i < signers.length; i++) {
+        for (uint i = 0; i < signers.length; i++) {
             if (isSigner[signers[i]]) {
                 require(activeSignersCount > 1, "must not remove last signer");
                 activeSignersCount--;
@@ -137,7 +144,7 @@ contract MultiSig {
     /* implement it in derived contract */
     function checkQuorum(uint signersCount) internal view returns(bool isQuorum);
 
-    function getAllSignersCount() view external returns (uint allSignersCount) {
+    function getAllSignersCount() external view returns (uint allSignersCount) {
         return allSigners.length;
     }
 
@@ -153,7 +160,7 @@ contract MultiSig {
         return response;
     }
 
-    function getScriptsCount() view external returns (uint scriptsCount) {
+    function getScriptsCount() external view returns (uint scriptsCount) {
         return scriptAddresses.length;
     }
 
@@ -170,4 +177,18 @@ contract MultiSig {
         }
         return response;
     }
+
+    function _execute(address scriptAddress) private returns (bool result) {
+        Script storage script = scripts[scriptAddress];
+        // passing scriptAddress to allow called script access its own public fx-s if needed
+        if (scriptAddress.delegatecall.gas(gasleft() - 23000)
+            (abi.encodeWithSignature("execute(address)", scriptAddress))) {
+            script.state = ScriptState.Done;
+            result = true;
+        } else {
+            script.state = ScriptState.Failed;
+            result = false;
+        }
+        emit ScriptExecuted(scriptAddress, result);
+    }
 }
diff --git a/contracts/scriptTests/SB_revertingNoReasonScript.sol b/contracts/scriptTests/SB_revertingNoReasonScript.sol
@@ -0,0 +1,15 @@
+/* test script to simulate revert() in script execute() to multiSig
+*/
+
+pragma solidity 0.4.24;
+
+
+contract SB_revertingNoReasonScript {
+
+    function execute(SB_revertingNoReasonScript self) pure external {
+        require(address(self) != 0, "just to silence unused var compiler warning");
+        // solium-disable-next-line error-reason
+        revert();
+    }
+
+}
diff --git a/test/stabilityBoardProxy.js b/test/stabilityBoardProxy.js
@@ -3,6 +3,7 @@ const SB_addSigners = artifacts.require("scriptTests/SB_addSigners.sol");
 const SB_addAndRemoveSigners = artifacts.require("scriptTests/SB_addAndRemoveSigners.sol");
 const SB_removeSigners = artifacts.require("scriptTests/SB_removeSigners.sol");
 const SB_revertingScript = artifacts.require("scriptTests/SB_revertingScript.sol");
+const SB_revertingNoReasonScript = artifacts.require("scriptTests/SB_revertingNoReasonScript.sol");
 const SB_outOfGasScript = artifacts.require("scriptTests/SB_outOfGasScript.sol");
 const SB_cancelScript = artifacts.require("scriptTests/SB_cancelScript.sol");
 const testHelpers = require("./helpers/testHelpers.js");
@@ -20,10 +21,12 @@ async function addSigners(newSigners) {
         stabilityBoardProxy.getSigners(0, CHUNK_SIZE)
     ]);
 
-    const signTxs = currentSigners.filter(signerTuple => !signerTuple[1].eq(0)).map(tuple => {
-        const signerAddress = "0x" + tuple[1].toString(16).padStart(40, "0");
-        return stabilityBoardProxy.sign(addSignerScript.address, { from: signerAddress });
-    });
+    const signTxs = currentSigners
+        .filter(signerTuple => !signerTuple[1].eq(0))
+        .map(tuple => {
+            const signerAddress = "0x" + tuple[1].toString(16).padStart(40, "0");
+            return stabilityBoardProxy.sign(addSignerScript.address, { from: signerAddress });
+        });
 
     const signCount = Math.floor(signTxs.length / 2) + 1;
     await Promise.all(signTxs.slice(0, signCount));
@@ -295,9 +298,7 @@ contract("StabilityBoardProxy", accounts => {
         // create , sign & execute a script to cancel our removeSignerScript
         const cancelScript = await SB_cancelScript.new(removeSignerScript.address);
         await Promise.all(
-            [...newSigners, accounts[0]].map(signer =>
-                stabilityBoardProxy.sign(cancelScript.address, { from: signer })
-            )
+            [...newSigners, accounts[0]].map(signer => stabilityBoardProxy.sign(cancelScript.address, { from: signer }))
         );
 
         const cancelTx = await stabilityBoardProxy.execute(cancelScript.address, { from: accounts[0] });
@@ -428,14 +429,16 @@ contract("StabilityBoardProxy", accounts => {
             stabilityBoardProxy.getScriptsCount().then(res => res.toNumber()),
             stabilityBoardProxy.getScripts(scriptCountBefore, CHUNK_SIZE)
         ]);
-        const scripts = scriptsArray.filter(item => !item[1].eq(0)).map(item => {
-            return {
-                index: item[0].toNumber(),
-                address: "0x" + item[1].toString(16).padStart(40, "0"),
-                state: item[2].toNumber(),
-                signCount: item[3].toNumber()
-            };
-        });
+        const scripts = scriptsArray
+            .filter(item => !item[1].eq(0))
+            .map(item => {
+                return {
+                    index: item[0].toNumber(),
+                    address: "0x" + item[1].toString(16).padStart(40, "0"),
+                    state: item[2].toNumber(),
+                    signCount: item[3].toNumber()
+                };
+            });
 
         assert.equal(scripts.length, 3);
         assert.equal(scriptsCountAfter, scriptCountBefore + 3);
@@ -471,4 +474,81 @@ contract("StabilityBoardProxy", accounts => {
     it("should not call removeSigners directly", async function() {
         testHelpers.expectThrow(stabilityBoardProxy.removeSigners([accounts[0]]));
     });
+
+    it("should dryExecute a script (success)", async function() {
+        const addSignerScript = await SB_addAndRemoveSigners.new();
+
+        try {
+            await stabilityBoardProxy.dryExecute(addSignerScript.address);
+            assert.fail("Should be rejected");
+        } catch (error) {
+            assert.include(error.message, "VM Exception while processing transaction: revert dryExecute success");
+        }
+
+        let [allSignersCountAfter, script] = await Promise.all([
+            stabilityBoardProxy.getAllSignersCount(),
+            stabilityBoardProxyWeb3Contract.methods.scripts(addSignerScript.address).call(),
+            testHelpers.assertNoEvents(stabilityBoardProxy, "ScriptExecuted")
+        ]);
+
+        assert.equal(script.state, scriptState.New);
+        assert.equal(allSignersCountAfter.toNumber(), 1);
+    });
+
+    it("should dryExecute a script (revert with reason)", async function() {
+        const revertingScript = await SB_revertingScript.new();
+
+        try {
+            await stabilityBoardProxy.dryExecute(revertingScript.address);
+            assert.fail("Should be rejected");
+        } catch (error) {
+            assert.include(
+                error.message,
+                "VM Exception while processing transaction: revert intentional revert for test"
+            );
+        }
+
+        let [script] = await Promise.all([
+            stabilityBoardProxyWeb3Contract.methods.scripts(revertingScript.address).call(),
+            testHelpers.assertNoEvents(stabilityBoardProxy, "ScriptExecuted")
+        ]);
+
+        assert.equal(script.state, scriptState.New);
+    });
+
+    it("should dryExecute a script (revert no reason)", async function() {
+        const revertingNoReasonScript = await SB_revertingNoReasonScript.new();
+
+        try {
+            await stabilityBoardProxy.dryExecute(revertingNoReasonScript.address, { gas: 200000 });
+            assert.fail("Should be rejected");
+        } catch (error) {
+            assert.equal(error.message, "VM Exception while processing transaction: revert");
+        }
+
+        let [script] = await Promise.all([
+            stabilityBoardProxyWeb3Contract.methods.scripts(revertingNoReasonScript.address).call(),
+            testHelpers.assertNoEvents(stabilityBoardProxy, "ScriptExecuted")
+        ]);
+
+        assert.equal(script.state, scriptState.New);
+    });
+
+    it("should dryExecute a script (out of gas)", async function() {
+        const outOfGasScript = await SB_outOfGasScript.new();
+
+        try {
+            await stabilityBoardProxy.dryExecute(outOfGasScript.address, { gas: 200000 });
+            assert.fail("Should be rejected");
+        } catch (error) {
+            assert.equal(error.message, "VM Exception while processing transaction: revert");
+        }
+
+        let [script] = await Promise.all([
+            stabilityBoardProxyWeb3Contract.methods.scripts(outOfGasScript.address).call(),
+            testHelpers.assertNoEvents(stabilityBoardProxy, "ScriptExecuted")
+        ]);
+
+        assert.equal(script.state, scriptState.New);
+    });
 });