[StorageReading] Invoke external viewer contract via static delegatecall #31

fleupold · 2020-05-29T16:39:11Z

While #30 works great for reading specific one-off storage slots, it is not very convenient in case we want to add more sophisticated view function logic to an already deployed contract (e.g. to improve gas efficiency for view functions on Gnosis Protocol)

This PR allows anyone to deploy a smart contract with the same storage layout as our original (StorageReading subclassed) contract and implement arbitrary view functions on it. They can then invoke executeStaticDelegatecall on the existing contract, which will execute their code in its own context via a delegatecall (using the state of the original contract). Whatever happens in this call is reverted in the end, making it side-effect-less (static).

The return value is returned as abi-encoded bytes for easier consumption in web3 as well as other smart contracts (encoding as revert message would suffice for the latter).

The cool thing is that we can even invoke methods that are non-view (i.e. have side effects) and thus simulate more complex invocations. All side effects will be reverted, the returned result remains.

This is similar to the state override feature in geth but make it client agnostic and available to other smart contracts.

Credit goes to @rmeissner who wrote this code before (I'm just making it generally accessible here)

Test Plan

Added unit test to demonstrate how a fixed contract can be amended with reader functionality.

…ntext of base contract

rmeissner

Looks very nice :)

Maybe having an example where the modified state is returned would be nice (as this is a cool feature that could be showcased ;) )

rmeissner · 2020-05-29T16:49:20Z

contracts/StorageAccessible.sol

+     * @param targetContract Address of the contract containing the code to execute.
+     * @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments).
+     */
+    function executeStaticDelegatecallInternal(


I find this name misleading (minimal) :D ... there is no "staticcall" anywhere (I know that it is very well documented in the natspec :D ) ... but not there if there is a better name.

How about simulateDelegatecall?

rmeissner · 2020-05-29T16:50:05Z

contracts/StorageAccessible.sol

+            calldataPayload
+        );
+        assembly {
+            revert(add(response, 0x20), mload(response))


Should we try to open a issue with the solidity team to get a way to do this without assembly (the revert of bytes)?

Clever! Do I understand this to mean that the response data is the revert message?

So a revert message works different. Solidity encoded the revert message like a call to a function Error(string) (see https://solidity.readthedocs.io/en/v0.5.3/control-structures.html#error-handling-assert-require-revert-and-exceptions). Currently it is not possible to return raw bytes (without any additional encoding). Therefore it is required to use assembly for this.

@fleupold if required I could open an issue on the solidity repo

So the issue would request support for calling revert with bytes as arguments inside solidity right? I'm not sure this is the intended use of revert (we are kind of shoehorning it a bit here) and thus I'm fine with using assembly for this.

Do you have another use case where we would legitimately want to return bytes from a revert?

Feel free to open issue regardless, we can see what the solidity community thinks about.

rmeissner · 2020-05-29T16:51:37Z

contracts/StorageAccessible.sol

+        bytes memory calldataPayload
+    ) public returns (bytes memory) {
+        bytes memory innerCall = abi.encodeWithSignature(
+            "executeStaticDelegatecallInternal(address,bytes)",


General question: is it more effective to hardcode the bytes, or does the compiler solve this? Might be worth considering it if this is only optimised when the optimiser is enabled.

Good point. Will make this a constant

Actually, since this call depends on calldata and targetContract I can't hardcode all of it. I can avoid doing the keccak hash an just append the arguments to it.

bh2smith

Very cool! I guess this will be something to use for v2 of the exchange contracts since they need to be StorageAccessible. Would there be any benefit to declaring something as explicitly private and demonstrating that the storage would still be "accessible"?

bh2smith · 2020-05-30T10:07:00Z

contracts/StorageAccessible.sol

+            calldataPayload
+        );
+        assembly {
+            revert(add(response, 0x20), mload(response))


Clever! Do I understand this to mean that the response data is the revert message?

bh2smith · 2020-05-30T10:08:27Z

contracts/test/StorageAccessibleWrapper.sol

+    function replaceFoo(uint256 foo_) public returns (uint256) {
+        uint256 tmp = foo;
+        foo = foo_;
+        return tmp;
+    }


Why do you want to return the old value of foo here? I feel like you can demonstrate the desired functionality by returning anything really.

Good point. Will change the example to reflect that!

bh2smith · 2020-05-30T10:14:08Z

test/storage_accessible.js

+      const reader = await ExternalStorageReader.new()
+      const replaceFooCall = reader.contract.methods.replaceFoo(69).encodeABI()
+      let result = await instance.executeStaticDelegatecall.call(reader.address, replaceFooCall)
+      assert.equal(42, fromHex(result))

-    const packed = await instance.getStorageAt(await instance.SLOT_FOOBAR(), 10)
-    assert.equal(19, fromHex(packed.slice(2, 66)))
-    assert.equal(21, fromHex(packed.slice(66, 130)))
+      // Make sure foo is not actually changed
+      const getFooCall = reader.contract.methods.getFoo().encodeABI()
+      result = await instance.executeStaticDelegatecall.call(reader.address, getFooCall)
+      assert.equal(42, fromHex(result))


OK, so the value isn't changed... I guess this means you can temporarily (i.e. within a single transaction) emulate what would or could happen in the event of a state change without every actually changing it... Is this what one might want it for?

Here is an example of we would like to do with it in the context of the safe contracts ;) https://github.com/gnosis/safe-contracts/blob/development/contracts/GnosisSafe.sol#L276

Cool example. If every smart contract developer picked up this technique, doesn’t this mean half the transactions submitted would be expected to fail?

anxolin

This is great! ❤️

josojo

Cool!

Did you consider writing the StorageAccess as a library instead of a contract?
If you would write it as a library, then you would not have to inherit the whole code, you would just need to add one "call function"

function allowStorageAccessLibraryReading(bytes calldata functionSignature, bytes[] calldata arguments) public returns (bytes memory){
libraryAddress.delegatecall(functionSignature, arguments)
}

This approach would make the deployment hopefully cheaper.

Too bad there is not a "view only" delegatecall, here it would be a nice fit for extra security.

second thought:
If you like inheriting, then you could also have a library and a very simple contract containing the allowStorageAccessLibraryReading function. Then, this contract would only have to be inherited.

fleupold · 2020-06-05T10:54:50Z

Did you consider writing the StorageAccess as a library instead of a contract?

@josojo I didn't consider using a library. I would like to avoid for people to write a custom function to expose this functionality. Your second suggestion would work for that, but if I understand correctly it would still require linking the dapp contract against the StorageReadingLibary (on top of inheriting).

I'm not sure if the gas cost for deployment is going to be the limiting factor here (in the end if you intend to use this library you are likely building a dapp which you don't intend to redeploy anytime soon). For use cases like the safe we'd be using a proxy in the first place.

nlordell

Awesome sauce! Maybe add an example of a view function reverting to document what happens in that case.

nlordell · 2020-06-05T15:45:26Z

contracts/StorageAccessible.sol

+            calldataPayload
+        );
+        (, bytes memory response) = address(this).call(innerCall);
+        return response;


Aren't there slight semantic differences between reverting and just returning an encoded revert message? Should you check the response for the Error(string) selector and revert here (to propagate the revert) or even encode if the call was successful or not?

I now added a test case for delegateCalling a view function that reverts internally. It does get treated as a top level revert since as I assume the returned bytes are still the same. This is the behavior you expected, right?

Are you concerned about some return flag that would be set differently?

This is the behavior you expected, right?

Kind of. So actually this behaviour is AFAIK node dependent. From what I remember from ethcontract, Ganache returns a JSON RPC error when an eth_call reverts but Geth doesn't. So I imagine Truffle is designed to work with both, and will see the Error(string) selector in the response bytes and just assume it is a revert (which is how Geth would behave anyway).

Are you concerned about some return flag that would be set differently?

So, I'm a bit more concerned about solidity code that calls simulateDelegatecall since it might subvert expectations that a revert in the targetContract call gets caught and not propagated. One use case that immediately comes to mind is writing an external contract (called A) to integrate with some contract (called B) that inherits from StorageAccessible and contract A needs to read some storage data from contract B for a transaction. The fact that simulateDelegatecall does not propagate reverts might cause bugs because the developers aren't expecting this behaviour.

You can also argue that this is the caller's responsibility, in that case I would maybe add a bit more documentation about this.

@nlordell can you take a look if 8a8f2ae adds the behavior you were expecting?

This is exactly the behaviour I was expecting.

Hmm, what happens if it revers without a reason?

Sorry, I think I'm getting carried away 😛 , I think the way it is now is 👌

Checkout 58f4ad2

This now also works with reverts that don't have a reason.

nlordell · 2020-06-05T16:06:12Z

test/storage_accessible.js

-    assert.equal(21, fromHex(packed.slice(66, 130)))
+      const reader = await ExternalStorageReader.new()
+      const doRevertCall = reader.contract.methods.doRevert().encodeABI()
+      truffleAssert.reverts(instance.simulateDelegatecall.call(reader.address), doRevertCall)


Shouldn't it be:

Suggested change

truffleAssert.reverts(instance.simulateDelegatecall.call(reader.address), doRevertCall)

truffleAssert.reverts(instance.simulateDelegatecall.call(reader.address, doRevertCall))

?

Totally. Great catch. Still passes.

This is one of those subtle issues that TypeScript would have helped find 😛

nlordell · 2020-06-12T14:46:16Z

contracts/StorageAccessible.sol

+            result.length > 4 &&
+            result[0] == 0x08 &&
+            result[1] == 0xc3 &&
+            result[2] == 0x79 &&
+            result[3] == 0xa0;


One tiny suggestion (but I'm not sure its worth doing) is maybe this check should be stricter, specifically:

First four bytes must be 0x08c379a0

Next 32-byte word must be 0x20 (string offset).

Read next 32-byte word as len

The result.length = 4 + k*32 such that k == ceil(len / 32) + 2

But that might be overkill.

Your approach is 100x better.

nlordell · 2020-06-12T15:52:42Z

contracts/StorageAccessible.sol

+            calldataPayload
+        );
+        (, bytes memory response) = address(this).call(innerCall);
+        bool innerSuccess = response[response.length - 1] == 0x01;


This is 🤘

Awesome!

nlordell

Looks awesome I have absolutely no more concerns!

[StorageReading] Allow external reader contract to execute code in co…

9766a5a

…ntext of base contract

fleupold requested review from cag, anxolin, rmeissner, nlordell, josojo and bh2smith May 29, 2020 16:39

rmeissner approved these changes May 29, 2020

View reviewed changes

bh2smith approved these changes May 30, 2020

View reviewed changes

anxolin approved these changes Jun 3, 2020

View reviewed changes

josojo approved these changes Jun 3, 2020

View reviewed changes

fleupold added 3 commits June 5, 2020 12:56

rename to simulateDelegatecall

1889e3a

hardcode selector

c691898

adjust test to show that we can simulate calls with side effects

863aafd

nlordell approved these changes Jun 5, 2020

View reviewed changes

Add test case for revert

f5a5210

nlordell reviewed Jun 5, 2020

View reviewed changes

fleupold added 4 commits June 5, 2020 18:11

@nlordell 🦅

b44163b

allow detection of reverts when invoked from other smart contract

8a8f2ae

add comment

be016ff

remove return in invokeDoRevertViaStorageAccessible

729d4b3

nlordell reviewed Jun 12, 2020

View reviewed changes

fleupold added 2 commits June 12, 2020 17:03

alternative implementation by propagating inner success flag

58f4ad2

adjust comment of internal method

71f7fcb

nlordell reviewed Jun 12, 2020

View reviewed changes

nlordell approved these changes Jun 12, 2020

View reviewed changes

fleupold merged commit fd08cb5 into master Jun 12, 2020

bh2smith deleted the static_delegatecall branch December 7, 2020 10:26

	truffleAssert.reverts(instance.simulateDelegatecall.call(reader.address), doRevertCall)
	truffleAssert.reverts(instance.simulateDelegatecall.call(reader.address, doRevertCall))

[StorageReading] Invoke external viewer contract via static delegatecall #31

[StorageReading] Invoke external viewer contract via static delegatecall #31

Conversation

fleupold commented May 29, 2020

Test Plan

rmeissner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmeissner Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bh2smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin left a comment

Choose a reason for hiding this comment

josojo left a comment • edited Loading

Choose a reason for hiding this comment

fleupold commented Jun 5, 2020

nlordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

rmeissner Jun 1, 2020 •

edited

Loading

josojo left a comment •

edited

Loading

nlordell Jun 5, 2020 •

edited

Loading

fleupold Jun 12, 2020 •

edited

Loading

nlordell Jun 12, 2020 •

edited

Loading