-
Notifications
You must be signed in to change notification settings - Fork 18
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
Changes from 4 commits
9766a5a
1889e3a
c691898
863aafd
f5a5210
b44163b
8a8f2ae
be016ff
729d4b3
58f4ad2
71f7fcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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; | ||
} | ||
|
||
/** | ||
* @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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 @fleupold if required I could open an issue on the solidity repo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the issue would request support for calling 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. |
||
} | ||
} | ||
} |
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)) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of. So actually this behaviour is AFAIK node dependent. From what I remember from
ethcontract
, Ganache returns a JSON RPC error when aneth_call
reverts but Geth doesn't. So I imagine Truffle is designed to work with both, and will see theError(string)
selector in the response bytes and just assume it is arevert
(which is how Geth would behave anyway).So, I'm a bit more concerned about solidity code that calls
simulateDelegatecall
since it might subvert expectations that arevert
in thetargetContract
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 fromStorageAccessible
and contract A needs to read some storage data from contract B for a transaction. The fact thatsimulateDelegatecall
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👌
There was a problem hiding this comment.
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.