solidity

solidity copied to clipboard

This is a weird one.

contract C0 {
  function f0() external returns (bytes memory o1)
  {
    assembly
    {
      for { let i := 0 } lt(i, 1) { i := add(i, 1) }
      {
        if o1 { break }
        o1 := keccak256(0, 12345678912)
      }
    }
  }
}
// ====
// compileViaYul: also
// ----
// f0() -> 0x20, 0

fails via Yul.

To repro

$ cp test.sol test/libsolidity/semanticTests
$ isoltest -t semanticTests/test --optimize --show-messages

Asm diff that I tried to make sense of but gave me :-(

https://gist.github.com/bshastry/0cbd6bb471f3b9c22ebf4b859669ce08

--- obtained from solc test.sol --experimental-via-ir --asm +++ obtained from solc test.sol --ir-optimized --asm

Another example

contract C0 {
  function f1() external returns (bytes memory o0)
  {
  assembly
    {
      {
          for { o0 := selfbalance() } o0
              {
                o0 := keccak256(0, 1155939646350332)
              } {}
      }
    }
  }
}
// ====
// compileViaYul: also
// ----
// f1() -> 0x20, 0

fails similarly via optimized IR.

This seems to be due to loop-invariant code motion. cc @hrkrshnn Maybe we should not pull out things that are potentially very expensive? On the other hand, the assumption was that if a loop is present in the code, we expect it to be executed at least once.

Yeah, the issue is with loop invariant code motion. The keccak is taken out of the loop, where it fails out of gas.

To be fair, there is no way to fix this without preventing mload, keccak from getting taken out of the loop. Especially if the offsets are not static. It's somewhat implicit that the optimizer assumes infinite gas, but this is a grey area, since EVM implementations seems to explicitly use 2**32 as max memory size and revert in all other cases. So not sure how we want to proceed here.

On the other hand, the assumption was that if a loop is present in the code, we expect it to be executed at least once.

Just to be precise, the assumption would be if loop present, all paths inside the loop body are executed at least once?

It's somewhat implicit that the optimizer assumes infinite gas, but this is a grey area, since EVM implementations seems to explicitly use 2**32 as max memory size and revert in all other cases.

The new codegen does assume a Max memory size, right?

See: https://github.com/ethereum/solidity/pull/11714

The new codegen does assume a Max memory size, right?

The optimizer doesn't assume this.

I'm fine with closing this as won't fix since I can't think of a strong enough threat model.

I was thinking along the lines of

"Malicious smart contract steals ether (gas) by placing keccak inside a for loop that would only be evaluated if the Yul optimizer is enabled."

But I'm afraid that's not a strong enough threat model.

This came up again while fuzzing the optimized EVM code transform

{
   function f()
   {
       for { } lt(0, 60) { }
       {
           {
               if xor(origin(), 0xff) { break }
               if mload(0x3fffffffff)
               {
                   revert(0, 0xf)
               }
           }
       }
   }
   f()
}

is optimised to

{
    let _1 := mload(0x3fffffffff)
    let _2 := iszero(eq(origin(), 0xff))
    for { } iszero(_2) { }
    { if _1 { revert(0, 0xf) } }
}

The unoptimised code does not run out of gas because the mload is not evaluated. The optimised code runs out of gas because mload is evaluated.

JFYI: This is reported quite often by the fuzzer.

In that case medium impact is probably warranted.

djeva

Does running the StructuralSimplifier (t) resolve the problem? Or is it too weak to detect that these conditions are false? It it can handle it, we could just consider it a recommended prerequisite of this step.

We should also mention this effect in LoopInvariantCodeMotion's docs.