Skip to content

Conversation

@msooseth
Copy link
Collaborator

@msooseth msooseth commented Sep 8, 2025

Description

As pointed out by Gustavo, we have actually missed forcing the value of the runAll. This lead to incorrect address being generated for e.g. the small contract with --only-deployed:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import {Test} from "forge-std/Test.sol";

contract C {
  function f() public pure returns (uint256) {
    return 42;
  }
}
contract T is Test {
  C c = new C();
  function prove_only_deployed_const(address x) pure public {
    uint256 y = C(x).f();
    assert(y != 42);
  }
}

The address should have been that of c, but instead, it was a random address. With this fix, the CEX validation system properly validates the CEX, i.e. the address is not random, but that of c.

This PR can only be merged on top of #827 -- currently failing because this PR is not yet merged

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@msooseth msooseth mentioned this pull request Sep 8, 2025
4 tasks
Adding changelog

Adding test that's wrong for the moment

Add test
@msooseth msooseth changed the title Adding forcing of value for runAll [DRAFT -- #827 required] Adding forcing of value for runAll Sep 8, 2025
@msooseth msooseth changed the title [DRAFT -- #827 required] Adding forcing of value for runAll Adding forcing of value for runAll Sep 9, 2025
@msooseth msooseth requested a review from blishko September 9, 2025 14:17
@msooseth msooseth marked this pull request as ready for review September 9, 2025 14:17
Copy link
Collaborator

@blishko blishko left a comment

Choose a reason for hiding this comment

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

Looks good! I just have a question about the test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really using PleaseRunAll? Isn't there just one possible value?
I just want to make sure we are testing what we want to test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, there are at least two known address here: the deployed C and T contracts. The origin addresses are also valid ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair point.

With current main, running: cabal run exe:hevm -- test --root tmp --match "only_deployed_force_addr" --only-deployed gives us:

[RUNNING] prove_only_deployed_force_addr(address)
   [FAIL] prove_only_deployed_force_addr
   Counterexample:   [not reproducible]
     calldata: prove_only_deployed_force_addr(0x0470C202ca30050800087234Ffee81fb7FCEaFc4)
     result:   Revert: 0x4e487b710000000000000000000000000000000000000000000000000000000000000001

This current branch gives:

[RUNNING] prove_only_deployed_force_addr(address)
   [FAIL] prove_only_deployed_force_addr
   Counterexample:   [validated]
     calldata: prove_only_deployed_force_addr(0xD95322745865822719164b1fC167930754c248DE)
     result:   Revert: 0x4e487b710000000000000000000000000000000000000000000000000000000000000001

The difference between the two is only this PR. The reason for this is that this runAll:

onlyDeployed addrExpr fallback continue = do
    vm <- get
    if not (?conf.onlyDeployed) then fallback addrExpr
    else case eqT @t @Symbolic of
      Just Refl -> do
        let deployedAddrs = map forceEAddrToEWord $ mapMaybe (codeMustExist vm) $ Map.keys vm.env.contracts
        runAll (?conf.maxDepth) vm.exploreDepth $ PleaseRunAll addrExpr deployedAddrs runAllPaths
      _ -> internalError "Unknown address in Concrete mode"

Will run for all --only-deployed as long as there are too many solutions. So it will run for this case, too, since the address can be anything here:

  function prove_only_deployed_force_addr(address x) pure public {
    uint256 y = C(x).f();
    assert(y != 42);
  }

There is no restriction on x. So the SMT solver gets 100 different soltuions and then it gives up, and falls back to onlyDeployed:

forceAddr addrExpr fallback continue = case wordToAddr addrExpr of
  Nothing -> manySolutions (?conf).maxDepth addrExpr 20 $ \case
    Just sol -> continue $ LitAddr (truncateToAddr sol)
    Nothing -> onlyDeployed addrExpr fallback continue
  Just c -> continue c

I hope this is a good explanation :) Took me a while to think through this but I hope it's a good description of what's going on!

@gustavo-grieco
Copy link
Collaborator

Is there any blocker to merge? There is a test in red, but I don't know it is related 🤔

@blishko blishko merged commit 87fe0ee into main Sep 14, 2025
6 of 7 checks passed
@blishko blishko deleted the fix-missing-force branch September 14, 2025 12:52
@blishko
Copy link
Collaborator

blishko commented Sep 14, 2025

The failing test is unrelated. We'll investigate later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants