welcome[bot]
commented
5 years ago Thanks for opening your first issue in aragonOS! Someone will circle back soon ⚡
Closed ritzdorf closed 5 years ago
welcome[bot]
commented
5 years ago Thanks for opening your first issue in aragonOS! Someone will circle back soon ⚡
sohkai
commented
5 years ago Hmm, this is problematic indeed. This has various ramifications throughout Aragon's existing smart contracts, the largest of which is an organization's Vault being able to receive ETH through simple contract transfers (e.g. Finance -> Vault deposit, Agent receiving ETH from CDPs, Compound, etc.).
Given that we are really close to the limit, we could optimize this a bit to have it fit inside 2300, but it presents a migration problem for all existing 600+ Aragon organizations deployed to date (and for the foreseeable future) as we can't upgrade the proxy contracts.
ritzdorf
commented
5 years ago Thank you for the explanation.
I agree that an optimization seems feasible. The easiest option might be to modify the event. Otherwise, the optimization becomes slightly harder. The current version of the event (counting only LOG1 not the necessary setup) consumes 1262 gas, together with the new cost of SLOAD (800) that is already 2062 gas spent.
holiman
commented
5 years ago it presents a migration problem for all existing 600+ Aragon organizations deployed to date (and for the foreseeable future) as we can't upgrade the proxy contracts.
Could you elaborate on this? I am not at all familiar with Aragon, really, so please outline how big of a problem, and what the impact or possible consequences could be.
izqui
commented
5 years ago @holiman the contracts that compose Aragon organizations use the DelegateProxy pattern for upgradeability. Thanks to this we can upgrade most of the logic of the contracts, except for the code in the proxy. The code that allows these contracts to receive ETH when sent with 2300 gas had to be implemented in the proxy code directly, as deferring this to the implementation code (performing the delegatecall) made it go over the 2300 gas limit.
The issue right now is that there are ~680 contracts (one per organization) that will no longer be able to receive ETH when sent from a smart contract, including receiving ETH from another Aragon organization, after EIP-1884 is activated.
The migration path for existing organizations would be quite tedious, as each organization would need to deploy a new proxy contract, modify sensitive permissions in the organization and move all the funds to this new proxy contract that can execute the fallback in under 2300 gas.
Because we don't have control over any of the deployed organizations, each of them would have to execute the process on their own. Even though we could build tools for this, it would be a painful process.
holiman
commented
5 years ago Ok. Is this a correct assumption?
2359 gas, in the affected code path"If so, it seems that this issue could be resolved by simply lowering the cost of LOG operations by a little bit. It would be interesting to see a trace of the execution.
ritzdorf
commented
5 years ago @holiman Here is the gzipped trace of the example execution mentioned above (0x14a4a43b4e9759aac86bb0ae7e5926850406ff1c43ea571239563ff781474ae0). The relevant fallback function gets executed on depth 8.
I don't know if all affected contracts will use exactly this amount of gas as this is the only one where I know the exact address so that I could check. I currently do not see how to efficiently get all the vault addresses from the explorer, otherwise I would be happy to check.
holiman
commented
5 years ago Here is the execution trace with nicer formatting. The trace for some reason contains the wrong values for gasCost for many opcodes, I have no idea why.
Part one:
╔════════════════════════════════════Operations═════════════════════════════════════╗│┌──────────────────────────────────────Op──────────────────────────────────────┐│
│║STEP PC OPNAME OPCODE GAS GASCOST DEPTH REFUND ║││ ││
│║12176 5094 (0x13e6) CALL 0xf1 164370 9700 7 0 ║││ pc 5094 (0x13e6) ││
│║12177 0 (0x0) PUSH1 0x60 2300 0 8 0 ║││ opcode 0xf1 ││
│║12178 2 (0x2) PUSH1 0x60 2297 0 8 0 ║││ opName CALL ││
│║12179 4 (0x4) MSTORE 0x52 2294 12 8 0 ║││ gasCost 9700 ││
│║12180 5 (0x5) PUSH1 0x60 2282 12 8 0 ║││ gas 164370 ││
│║12181 7 (0x7) CALLDATASIZE 0x36 2279 12 8 0 ║││ memSize 0 ││
│║12182 8 (0x8) LT 0x10 2277 12 8 0 ║││ addr NA ││
│║12183 9 (0x9) PUSH2 0x61 2274 12 8 0 ║││ Pops gas,address,value,in offset,in size,out offset,out size ││
│║12184 12 (0xc) JUMPI 0x57 2271 12 8 0 ║││ Pushes exitcode (1 for success) ││
│║12185 108 (0x6c) JUMPDEST 0x5b 2261 12 8 0 ║││ ││
│║12186 109 (0x6d) PUSH1 0x60 2260 12 8 0 ║││ ││
│║12187 111 (0x6f) PUSH2 0x61 2257 12 8 0 ║││ ││
│║12188 114 (0x72) GAS 0x5a 2254 12 8 0 ║││ ││
│║12189 115 (0x73) LT 0x10 2252 12 8 0 ║││ ││
│║12190 116 (0x74) ISZERO 0x15 2249 12 8 0 ║││ ││
│║12191 117 (0x75) PUSH2 0x61 2246 12 8 0 ║││ ││
│║12192 120 (0x78) JUMPI 0x57 2243 12 8 0 ║││ ││
│║12193 121 (0x79) PUSH1 0x60 2233 12 8 0 ║││ ││
│║12194 123 (0x7b) CALLVALUE 0x34 2230 12 8 0 ║││ ││
│║12195 124 (0x7c) GT 0x11 2228 12 8 0 ║││ ││
│║12196 125 (0x7d) DUP1 0x80 2225 12 8 0 ║││ ││
│║12197 126 (0x7e) ISZERO 0x15 2222 12 8 0 ║││ ││
│║12198 127 (0x7f) PUSH2 0x61 2219 12 8 0 ║││ ││
│║12199 130 (0x82) JUMPI 0x57 2216 12 8 0 ║││ ││
│║12200 131 (0x83) POP 0x50 2206 12 8 0 ║││ ││
│║12201 132 (0x84) CALLDATASIZE 0x36 2204 12 8 0 ║││ ││
│║12202 133 (0x85) ISZERO 0x15 2202 12 8 0 ║││ ││
│║12203 134 (0x86) JUMPDEST 0x5b 2199 12 8 0 ║││ ││
│║12204 135 (0x87) ISZERO 0x15 2198 12 8 0 ║││ ││
│║12205 136 (0x88) ISZERO 0x15 2195 12 8 0 ║││ ││
│║12206 137 (0x89) PUSH2 0x61 2192 12 8 0 ║││ ││
│║12207 140 (0x8c) JUMPI 0x57 2189 12 8 0 ║││ ││
│║12208 145 (0x91) JUMPDEST 0x5b 2179 12 8 0 ║││ ││
│║12209 146 (0x92) PUSH2 0x61 2178 12 8 0 ║││ ││
│║12210 149 (0x95) PUSH2 0x61 2175 12 8 0 ║││ ││
│║12211 152 (0x98) JUMP 0x56 2172 12 8 0 ║││ ││
│║12212 483 (0x1e3) JUMPDEST 0x5b 2164 12 8 0 ║││ ││
│║12213 484 (0x1e4) PUSH1 0x60 2163 12 8 0 ║││ ││
│║12214 486 (0x1e6) PUSH2 0x61 2160 12 8 0 ║││ ││
│║12215 489 (0x1e9) PUSH32 0x7f 2157 12 8 0 ║││ ││
│║12216 522 (0x20a) PUSH2 0x61 2154 12 8 0 ║││ ││
│║12217 525 (0x20d) JUMP 0x56 2151 12 8 0 ║││ ││
│║12218 701 (0x2bd) JUMPDEST 0x5b 2143 12 8 0 ║││ ││
│║12219 702 (0x2be) SLOAD 0x54 2142 200 8 0 ║││ ││
│╚═══════════════════════════════════════════════════════════════════════════════════╝│└──────────────────────────────────────────────────────────────────────────────┘│
├─────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│┌───────────────────────────────────────Stack───────────────────────────────────────┐│┌────────────────────────────────────Memory────────────────────────────────────┐│
││POS DATA DESC │││ 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f [ -- ascii -- ] ││
││00 0 gas │││00000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││01 a74d136fafed0f8d58ce4b7307283695ec7a0b6 address │││00000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││02 10a741a462780000 value │││00000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││03 180 in offset │││00000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││04 0 in size │││00000040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││05 180 out offset │││00000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 80 ................ ││
││06 0 out size │││00000060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││07 180 │││00000070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││08 10a741a462780000 │││00000080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││09 0 │││00000090 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 60 ...............` ││
││10 a74d136fafed0f8d58ce4b7307283695ec7a0b6 │││000000a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││11 80 │││000000b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
│└───────────────────────────────────────────────────────────────────────────────────┘│└──────────────────────────────────────────────────────────────────────────────┘│Part two:
╔════════════════════════════════════Operations═════════════════════════════════════╗│┌──────────────────────────────────────Op──────────────────────────────────────┐│
│║STEP PC OPNAME OPCODE GAS GASCOST DEPTH REFUND ║││ ││
│║12219 702 (0x2be) SLOAD 0x54 2142 200 8 0 ║││ pc 298 (0x12a) ││
│║12220 703 (0x2bf) SWAP1 0x90 1942 200 8 0 ║││ opcode 0x0 ││
│║12221 704 (0x2c0) JUMP 0x56 1939 200 8 0 ║││ opName STOP ││
│║12222 526 (0x20e) JUMPDEST 0x5b 1931 200 8 0 ║││ gasCost 1262 ││
│║12223 527 (0x20f) SWAP1 0x90 1930 200 8 0 ║││ gas 541 ││
│║12224 528 (0x210) POP 0x50 1927 200 8 0 ║││ memSize 0 ││
│║12225 529 (0x211) SWAP1 0x90 1925 200 8 0 ║││ addr NA ││
│║12226 530 (0x212) JUMP 0x56 1922 200 8 0 ║││ Pops ││
│║12227 153 (0x99) JUMPDEST 0x5b 1914 200 8 0 ║││ Pushes ││
│║12228 154 (0x9a) ISZERO 0x15 1913 200 8 0 ║││ ││
│║12229 155 (0x9b) ISZERO 0x15 1910 200 8 0 ║││ ││
│║12230 156 (0x9c) PUSH2 0x61 1907 200 8 0 ║││ ││
│║12231 159 (0x9f) JUMPI 0x57 1904 200 8 0 ║││ ││
│║12232 164 (0xa4) JUMPDEST 0x5b 1894 200 8 0 ║││ ││
│║12233 165 (0xa5) PUSH1 0x60 1893 200 8 0 ║││ ││
│║12234 167 (0xa7) DUP1 0x80 1890 200 8 0 ║││ ││
│║12235 168 (0xa8) MLOAD 0x51 1887 3 8 0 ║││ ││
│║12236 169 (0xa9) CALLER 0x33 1884 3 8 0 ║││ ││
│║12237 170 (0xaa) DUP2 0x81 1882 3 8 0 ║││ ││
│║12238 171 (0xab) MSTORE 0x52 1879 9 8 0 ║││ ││
│║12239 172 (0xac) CALLVALUE 0x34 1870 9 8 0 ║││ ││
│║12240 173 (0xad) PUSH1 0x60 1868 9 8 0 ║││ ││
│║12241 175 (0xaf) DUP3 0x82 1865 9 8 0 ║││ ││
│║12242 176 (0xb0) ADD 0x1 1862 9 8 0 ║││ ││
│║12243 177 (0xb1) MSTORE 0x52 1859 6 8 0 ║││ ││
│║12244 178 (0xb2) DUP2 0x81 1853 6 8 0 ║││ ││
│║12245 179 (0xb3) MLOAD 0x51 1850 3 8 0 ║││ ││
│║12246 180 (0xb4) PUSH32 0x7f 1847 3 8 0 ║││ ││
│║12247 213 (0xd5) SWAP3 0x92 1844 3 8 0 ║││ ││
│║12248 214 (0xd6) SWAP2 0x91 1841 3 8 0 ║││ ││
│║12249 215 (0xd7) DUP2 0x81 1838 3 8 0 ║││ ││
│║12250 216 (0xd8) SWAP1 0x90 1835 3 8 0 ║││ ││
│║12251 217 (0xd9) SUB 0x3 1832 3 8 0 ║││ ││
│║12252 218 (0xda) SWAP1 0x90 1829 3 8 0 ║││ ││
│║12253 219 (0xdb) SWAP2 0x91 1826 3 8 0 ║││ ││
│║12254 220 (0xdc) ADD 0x1 1823 3 8 0 ║││ ││
│║12255 221 (0xdd) SWAP1 0x90 1820 3 8 0 ║││ ││
│║12256 222 (0xde) LOG1 0xa1 1817 1262 8 0 ║││ ││
│║12257 223 (0xdf) PUSH2 0x61 555 1262 8 0 ║││ ││
│║12258 226 (0xe2) JUMP 0x56 552 1262 8 0 ║││ ││
│║12259 296 (0x128) JUMPDEST 0x5b 544 1262 8 0 ║││ ││
│║12260 297 (0x129) POP 0x50 543 1262 8 0 ║││ ││
│║12261 298 (0x12a) STOP 0x0 541 1262 8 0 ║││ ││
│║12262 5095 (0x13e7) SWAP4 0x93 155211 9700 7 0 ║││ ││
│╚═══════════════════════════════════════════════════════════════════════════════════╝│└──────────────────────────────────────────────────────────────────────────────┘│The bulk of the cost is from
LOG1 0xa1 1817 1262The LOG1 operation details:
│┌───────────────────────────────────────Stack───────────────────────────────────────┐│┌────────────────────────────────────Memory────────────────────────────────────┐│
││POS DATA DESC │││ 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f [ -- ascii -- ] ││
││00 80 mStart │││00000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││01 40 mSize │││00000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││02 15eeaa57c7bd188c1388020bcadc2c436ec60d647d36ef5b9eb3c742217ddee1 topic │││00000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││03 0 │││00000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││ │││00000040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││ │││00000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80 ................ ││
││ │││00000060 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││ │││00000070 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││ │││00000080 00 00 00 00 00 00 00 00 00 00 00 00 e2 7c af 89 .............|.. ││
││ │││00000090 51 7a 33 da be 83 6b b7 40 ff 96 cc 37 51 a0 86 Qz3...k.@...7Q.. ││
││ │││000000a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ ││
││ │││000000b0 00 00 00 00 00 00 00 00 10 a7 41 a4 62 78 00 00 ..........A.bx.. ││
│└───────────────────────────────────────────────────────────────────────────────────┘│└──────────────────────────────────────────────────────────────────────────────┘So it uses 1 topic, and 0x40 bytes of memory. There is no memory expansion, but 375 + 1 * 375 (topics) + 0x40 * 8 (logdata) = 1262 gas.
sohkai
commented
5 years ago Ok. Is this a correct assumption?
"all of those affected contracts will use exactly 2359 gas, in the affected code path"
Yes, this should be correct.
I currently do not see how to efficiently get all the vault addresses from the explorer, otherwise I would be happy to check.
If this is necessary we can send a list of addresses, although they will all act the same as the one you used.
tkstanczak
commented
5 years ago pasting here the solution I proposed on AllCoreDev(for reference) I also think that in this particular scenario LOG, LOG_TOPIC cost reduction to 300, 375 would suffice
EIP-1884 Solution 1) (stipend fix only)
The idea is to use the fact that when calling with a non-zero value, 9000 gas is used instead of 700. Moreover this 9000 is calculated as a cost of the caller and callee account balance updates (two state updates). Since these accounts will be cached by the clients, an attacker would have to keep calling a different predeployed contract each time to take advantage of the discounts proposed below in a loop and (in order to cause clients to load and update non-cached accounts). Attacker would have to pay 9000 for each call and significant amount of gas for each of the target contract deployments before the attack and additional gas for storing the target contract addresses in the calling contract storage before executing an attack. In such scenario, the attack should be prohibitively expensive.
(in such attack we get 9000 (non-zero value call) - 700 (standard call cost) gas for free to minimize the impact of the proposed stipend discount.
Let us introduce a short (2-byte) STIPEND_USE counter on EVM state (state at the current EVM call). Let us introduce a boolean STIPEND_AVAILABLE variable on EVM state (for better performance of gas updates).
At the beginning of any call set STIPEND_AVAILABLE to true when TRANSFER_VALUE > 0 && CALL_DEPTH > 0.
STIPEND_USE starts at 0 each time GAS_USED is increased if (STIPEND_AVAILABLE) increase the STIPEND_USE by the same amount then if STIPEND_USE >= 2300 SET STIPEND_AVAILABLE to false if (!STIPEND_AVAILABLE) do nothing Now set price of SLOAD to:
800 if NOT(STIPEND_AVAILABLE) 200 otherwise Now set price of BALANCE to:
700 if NOT(STIPEND_AVAILABLE) 400 otherwise Now set price of EXTCODEHASH to:
700 if NOT(STIPEND_AVAILABLE) 400 otherwise
The client memory cost is ~3 bytes per call depth The client performance cost is a ulong to short cast + short addition + short comparison when STIPEND_USE is below 2300, then one boolean check for each gas update Maintenance cost - amortized since we can use this solution for any repricing in the future, additional logic in GAS_USED updates.
ritzdorf
commented
5 years ago @holiman's proposal of lowering the cost of LOG seems to be a very convenient solution for this particular contract.
@holiman : I don't see LOG as risky in your vmstats analysis. So was LOG currently overpriced?
MicahZoltu
commented
5 years ago Regardless of the immediate term solution to this problem, dapp developers (e.g., Aragon) should not assume that gas costs will remain constant over time. They are designed to change as operational costs of various operations change over time relative to each other. You should not be using .transfer and .send and instead protect against reentrancy via other patterns.
On top of that, .transfer and .send do not give enough of a gas stipend for smart wallets to execute advanced functionality on receipt, so along with the fact that they may break in some future fork, they currently break when interacting with any smart wallet that is actually smart (e.g., a smart wallet that proxies funds on receipt).
holiman
commented
5 years ago they currently break when interacting with any smart wallet that is actually smart (e.g., a smart wallet that proxies funds on receipt).
Well, it's not been easy for contract devs. This is also considered an anti-pattern, where the more recommended route has been to let the final recipient(s) pull, instead of "pushing" it to them.
An underlying problem is that there hasn't existed an CALL type that means "no state changes but log is allowed". Otherwise "STATICCALLWITHLOGS" could have been a great primitive for transfer
holiman
commented
5 years ago @holiman : I don't see LOG as risky in your vmstats analysis. So was LOG currently overpriced?
So LOG is one of those which aren't well covered by vmstats, like SSTORE. They have post-execution side-effects, which aren't captured at runtime. So their cost needs to take that into account, not just the speed of executing the op.
MicahZoltu
commented
5 years ago Yes, I'm a big fan of the mailbox pattern and encourage its use when it makes sense to solve the problem. This way you can let the transfer happen with as much gas as the recipient needs to do whatever accounting it wants to on receipt. I don't know enough about this particular situation in Aragon to know whether that is an appropriate solution here, but I do think something along those lines should be used instead of gas stipends.
Background
The upcoming Istanbul Ethereum hardfork contains the EIP-1884. EIP-1884 will, besides other things, raise the gas cost of the EVM
SLOADinstruction from 200 to 800 gas.Issue Description
The fallback function of
DepositableDelegateProxy:and the relevant
isDepositable()function:Basically, as the code comment says, in case the fallback function was triggered by a
.send()or.transfer(), which only send along 2300 gas, the first branch will be executed. Then,isDepositable()will be executed and aProxyDepositevent will be emitted.Currently, the execution of this function consumes 1759 gas. During the execution one
SLOADis executed due toisDepositable(). Hence, the gas cost after the hardfork will be1759 + 600 = 2359which will run out of gas. Hence, the transaction will generally revert.Example
The contract is deployed at address 0x0a74d136fafed0f8d58ce4b7307283695ec7a0b6 and its fallback function was recently executed with 2300 gas as part of the transaction 0x14a4a43b4e9759aac86bb0ae7e5926850406ff1c43ea571239563ff781474ae0.
Additional Information
ChainSecurity has conducted some analysis on the effects of EIP-1884. The intermediate results can be found here.
Martin Holst Swende has collected information on the effects of EIP-1884 here.
Contact
Feel free to contact hubert@chainsecurity.com or
@HRitzdorfon Telegram in case of questions or comments.