Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Jun 12, 2020
41 changes: 41 additions & 0 deletions contracts/StorageAccessible.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ pragma solidity ^0.5.2;

/// @title StorageAccessible - generic base contract that allows callers to access all internal storage.
contract StorageAccessible {
bytes4 public constant SIMULATE_DELEGATECALL_INTERNAL_SELECTOR = bytes4(
keccak256("simulateDelegatecallInternal(address,bytes)")
);

/**
* @dev Reads `length` bytes of storage in the currents contract
* @param offset - the offset in the current contract's storage in words to start reading from
Expand All @@ -23,4 +27,41 @@ contract StorageAccessible {
}
return result;
}

/**
* @dev Performs a delegetecall on a targetContract in the context of self.
* Internally reverts execution to avoid side effects (making it static). Catches revert and returns encoded result as bytes.
* @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 simulateDelegatecall(
address targetContract,
bytes memory calldataPayload
) public returns (bytes memory) {
bytes memory innerCall = abi.encodeWithSelector(
SIMULATE_DELEGATECALL_INTERNAL_SELECTOR,
targetContract,
calldataPayload
);
(, bytes memory response) = address(this).call(innerCall);
return response;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@nlordell nlordell Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly the behaviour I was expecting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what happens if it revers without a reason?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@fleupold fleupold Jun 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkout 58f4ad2

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

}

/**
* @dev Performs a delegetecall on a targetContract in the context of self.
* Internally reverts execution to avoid side effects (making it static). Returns encoded result as revert message.
* @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 simulateDelegatecallInternal(
address targetContract,
bytes memory calldataPayload
) public returns (bytes memory) {
(, bytes memory response) = targetContract.delegatecall(
calldataPayload
);
assembly {
revert(add(response, 0x20), mload(response))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@rmeissner rmeissner Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
19 changes: 19 additions & 0 deletions contracts/test/StorageAccessibleWrapper.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,22 @@ contract StorageAccessibleWrapper is StorageAccessible {
foobar = FooBar({foo: foo_, bar: bar_});
}
}


/**
* Defines reader methods on StorageAccessibleWrapper that can be later executed
* in the context of a previously deployed instance
*/
contract ExternalStorageReader {
// Needs same storage layout as the contract it is reading from
uint256 foo;

function getFoo() public view returns (uint256) {
return foo;
}

function setAndGetFoo(uint256 foo_) public returns (uint256) {
foo = foo_;
return foo;
}
}
98 changes: 65 additions & 33 deletions test/storage_accessible.js
Original file line number Diff line number Diff line change
@@ -1,51 +1,83 @@
const StorageAccessibleWrapper = artifacts.require('StorageAccessibleWrapper')
const ExternalStorageReader = artifacts.require('ExternalStorageReader')

contract('StorageAccessible', () => {
const fromHex = string => parseInt(string, 16)
const keccak = numbers => web3.utils.soliditySha3(
...numbers.map(v => ({ type: 'uint256', value: v }))
)

it('can read statically sized words', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setFoo(42)
describe('getStorageAt', async () => {
it('can read statically sized words', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setFoo(42)

assert.equal(42, await instance.getStorageAt(await instance.SLOT_FOO(), 1))
})
it('can read fields that are packed into single storage slot', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setBar(7)
await instance.setBam(13)
assert.equal(42, await instance.getStorageAt(await instance.SLOT_FOO(), 1))
})
it('can read fields that are packed into single storage slot', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setBar(7)
await instance.setBam(13)

const data = await instance.getStorageAt(await instance.SLOT_BAR(), 1)
const data = await instance.getStorageAt(await instance.SLOT_BAR(), 1)

assert.equal(7, fromHex(data.slice(34, 66)))
assert.equal(13, fromHex(data.slice(18, 34)))
})
it('can read arrays in one go', async () => {
const instance = await StorageAccessibleWrapper.new()
const slot = await instance.SLOT_BAZ()
await instance.setBaz([1, 2])
assert.equal(7, fromHex(data.slice(34, 66)))
assert.equal(13, fromHex(data.slice(18, 34)))
})
it('can read arrays in one go', async () => {
const instance = await StorageAccessibleWrapper.new()
const slot = await instance.SLOT_BAZ()
await instance.setBaz([1, 2])

const length = await instance.getStorageAt(slot, 1)
assert.equal(fromHex(length), 2)
const length = await instance.getStorageAt(slot, 1)
assert.equal(fromHex(length), 2)

const data = await instance.getStorageAt(keccak([slot]), length)
assert.equal(1, fromHex(data.slice(2, 66)))
assert.equal(2, fromHex(data.slice(66, 130)))
})
it('can read mappings', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setQuxKeyValue(42, 69)
assert.equal(69, await instance.getStorageAt(keccak([42, await instance.SLOT_QUX()]), 1))
const data = await instance.getStorageAt(keccak([slot]), length)
assert.equal(1, fromHex(data.slice(2, 66)))
assert.equal(2, fromHex(data.slice(66, 130)))
})
it('can read mappings', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setQuxKeyValue(42, 69)
assert.equal(69, await instance.getStorageAt(keccak([42, await instance.SLOT_QUX()]), 1))
})

it('can read structs', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setFoobar(19, 21)

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)))
})
})

it('can read structs', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setFoobar(19, 21)
describe('simulateDelegatecall', async () => {
it('can invoke a function in the context of a previously deployed contract', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setFoo(42)

// Deploy and use reader contract to access foo
const reader = await ExternalStorageReader.new()
const getFooCall = reader.contract.methods.getFoo().encodeABI()
const result = await instance.simulateDelegatecall.call(reader.address, getFooCall)
assert.equal(42, fromHex(result))
})

it('can simulate a function with side effects (without executing)', async () => {
const instance = await StorageAccessibleWrapper.new()
await instance.setFoo(42)

// Deploy and use reader contract to simulate setting foo
const reader = await ExternalStorageReader.new()
const replaceFooCall = reader.contract.methods.setAndGetFoo(69).encodeABI()
let result = await instance.simulateDelegatecall.call(reader.address, replaceFooCall)
assert.equal(69, 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.simulateDelegatecall.call(reader.address, getFooCall)
assert.equal(42, fromHex(result))
})
})
})