Skip to content

Commit 367aca9

Browse files
rmeissnerrmeissner
authored
Update StorageAccessible implementation (#290)
1 parent 39b69e2 commit 367aca9

File tree

3 files changed

+81
-44
lines changed

3 files changed

+81
-44
lines changed

‎contracts/common/StorageAccessible.sol‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
pragma solidity >=0.7.0 <0.9.0;
33

44
/// @title StorageAccessible - generic base contract that allows callers to access all internal storage.
5-
/// @notice Adjusted version of https://github.com/gnosis/util-contracts/blob/3db1e531cb243a48ea91c60a800d537c1000612a/contracts/StorageAccessible.sol
5+
/// @notice See https://github.com/gnosis/util-contracts/blob/bb5fe5fb5df6d8400998094fb1b32a178a47c3a1/contracts/StorageAccessible.sol
66
contract StorageAccessible {
77
/**
88
* @dev Reads `length` bytes of storage in the currents contract
@@ -27,22 +27,34 @@ contract StorageAccessible {
2727

2828
/**
2929
* @dev Performs a delegetecall on a targetContract in the context of self.
30-
* Internally reverts execution to avoid side effects (making it static). Returns encoded result as revert message
31-
* concatenated with the success flag of the inner call as a last byte.
30+
* Internally reverts execution to avoid side effects (making it static).
31+
*
32+
* This method reverts with data equal to `abi.encode(bool(success), bytes(response))`.
33+
* Specifically, the `returndata` after a call to this method will be:
34+
* `success:bool || response.length:uint256 || response:bytes`.
35+
*
3236
* @param targetContract Address of the contract containing the code to execute.
3337
* @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments).
3438
*/
35-
function simulate(
39+
function simulateDelegatecallInternal(
3640
address targetContract,
37-
bytes calldata calldataPayload
38-
) external returns (bytes memory) {
39-
(bool success, bytes memory response) = targetContract.delegatecall(
40-
calldataPayload
41-
);
42-
bytes memory responseWithStatus = abi.encodePacked(response, success);
43-
// solium-disable-next-line security/no-inline-assembly
41+
bytes memory calldataPayload
42+
) external {
43+
// solhint-disable-next-line no-inline-assembly
4444
assembly {
45-
revert(add(responseWithStatus, 0x20), mload(responseWithStatus))
45+
let success := delegatecall(
46+
gas(),
47+
targetContract,
48+
add(calldataPayload, 0x20),
49+
mload(calldataPayload),
50+
0,
51+
0
52+
)
53+
54+
mstore(0x00, success)
55+
mstore(0x20, returndatasize())
56+
returndatacopy(0x40, 0, returndatasize())
57+
revert(0, add(returndatasize(), 0x40))
4658
}
4759
}
4860
}

‎contracts/handler/CompatibilityFallbackHandler.sol‎

Lines changed: 52 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
4646
* @param _dataHash Hash of the data signed on the behalf of address(msg.sender)
4747
* @param _signature Signature byte array associated with _dataHash
4848
* @return a bool upon valid or invalid signature with corresponding _dataHash
49+
* @notice See https://github.com/gnosis/util-contracts/blob/bb5fe5fb5df6d8400998094fb1b32a178a47c3a1/contracts/StorageAccessible.sol
4950
*/
5051
function isValidSignature(bytes32 _dataHash, bytes calldata _signature)
5152
external
@@ -77,35 +78,59 @@ contract CompatibilityFallbackHandler is DefaultCallbackHandler, ISignatureValid
7778
* @param calldataPayload Calldata that should be sent to the target contract (encoded method name and arguments).
7879
*/
7980
function simulateDelegatecall(
80-
address targetContract,
81-
bytes calldata calldataPayload
82-
) external returns (bytes memory response) {
83-
bytes memory innerCall = abi.encodeWithSelector(
84-
SIMULATE_SELECTOR,
85-
targetContract,
86-
calldataPayload
87-
);
88-
(, response) = address(msg.sender).call(innerCall);
89-
bool innerSuccess = response[response.length - 1] == 0x01;
90-
setLength(response, response.length - 1);
91-
if (innerSuccess) {
92-
return response;
93-
} else {
94-
revertWith(response);
95-
}
96-
}
97-
98-
function revertWith(bytes memory response) internal pure {
99-
// solium-disable-next-line security/no-inline-assembly
81+
address targetContract, // solhint-disable-line no-unused-var
82+
bytes calldata calldataPayload // solhint-disable-line no-unused-var
83+
) public returns (bytes memory response) {
84+
// solhint-disable-next-line no-inline-assembly
10085
assembly {
101-
revert(add(response, 0x20), mload(response))
102-
}
103-
}
86+
let internalCalldata := mload(0x40)
87+
// Store `simulateDelegatecallInternal.selector`.
88+
mstore(internalCalldata, "\x43\x21\x8e\x19")
89+
// Abuse the fact that both this and the internal methods have the
90+
// same signature, and differ only in symbol name (and therefore,
91+
// selector) and copy calldata directly. This saves us approximately
92+
// 250 bytes of code and 300 gas at runtime over the
93+
// `abi.encodeWithSelector` builtin.
94+
calldatacopy(
95+
add(internalCalldata, 0x04),
96+
0x04,
97+
sub(calldatasize(), 0x04)
98+
)
10499

105-
function setLength(bytes memory buffer, uint256 length) internal pure {
106-
// solium-disable-next-line security/no-inline-assembly
107-
assembly {
108-
mstore(buffer, length)
100+
// `pop` is required here by the compiler, as top level expressions
101+
// can't have return values in inline assembly. `call` typically
102+
// returns a 0 or 1 value indicated whether or not it reverted, but
103+
// since we know it will always revert, we can safely ignore it.
104+
pop(call(
105+
gas(),
106+
// address() has been change to caller() to use the implemtation of the Safe
107+
caller(),
108+
0,
109+
internalCalldata,
110+
calldatasize(),
111+
// The `simulateDelegatecallInternal` call always reverts, and
112+
// instead encodes whether or not it was successful in the return
113+
// data. The first 32-byte word of the return data contains the
114+
// `success` value, so write it to memory address 0x00 (which is
115+
// reserved Solidity scratch space and OK to use).
116+
0x00,
117+
0x20
118+
))
119+
120+
121+
// Allocate and copy the response bytes, making sure to increment
122+
// the free memory pointer accordingly (in case this method is
123+
// called as an internal function). The remaining `returndata[0x20:]`
124+
// contains the ABI encoded response bytes, so we can just write it
125+
// as is to memory.
126+
let responseSize := sub(returndatasize(), 0x20)
127+
response := mload(0x40)
128+
mstore(0x40, add(response, responseSize))
129+
returndatacopy(response, 0x20, responseSize)
130+
131+
if iszero(mload(0x00)) {
132+
revert(add(response, 0x20), mload(response))
133+
}
109134
}
110135
}
111136
}

‎test/core/GnosisSafe.StorageAccessible.spec.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,26 +40,26 @@ describe("StorageAccessible", async () => {
4040
})
4141
})
4242

43-
describe("simulate", async () => {
43+
describe("simulateDelegatecallInternal", async () => {
4444

4545
it('should revert changes', async () => {
4646
const { safe, killLib } = await setupTests()
4747
await expect(
48-
safe.callStatic.simulate(killLib.address, killLib.interface.encodeFunctionData("killme"))
48+
safe.callStatic.simulateDelegatecallInternal(killLib.address, killLib.interface.encodeFunctionData("killme"))
4949
).to.be.reverted
5050
})
5151

5252
it('should revert the revert with message', async () => {
5353
const { safe, killLib } = await setupTests()
5454
await expect(
55-
safe.callStatic.simulate(killLib.address, killLib.interface.encodeFunctionData("trever"))
56-
).to.revertedWith("Why are you doing this?")
55+
safe.callStatic.simulateDelegatecallInternal(killLib.address, killLib.interface.encodeFunctionData("trever"))
56+
).to.be.reverted
5757
})
5858

5959
it('should return estimate in revert', async () => {
6060
const { safe, killLib } = await setupTests()
6161
await expect(
62-
safe.callStatic.simulate(killLib.address, killLib.interface.encodeFunctionData("estimate", [safe.address, "0x"]))
62+
safe.callStatic.simulateDelegatecallInternal(killLib.address, killLib.interface.encodeFunctionData("estimate", [safe.address, "0x"]))
6363
).to.be.reverted
6464
})
6565
})

0 commit comments

Comments
 (0)