Consensys / scribble

Scribble instrumentation tool
Apache License 2.0
314 stars 29 forks source link

Scribble crashes when interposing on public variable accessor calls #75

Closed cd1m0 closed 3 years ago

cd1m0 commented 3 years ago

For the following sample:

contract A {
  uint public a;
}

/// #invariant uint24(1) == 2;
contract B {
  function f() external returns (uint) {
     return A(msg.sender).a();
  }
}

Scribble crashes with the following stack:

 /home/dimo/work/consensys/scribble/dist/bin/scribble.js:484
            throw e;
            ^

Error
    at Object.assert (/home/dimo/work/consensys/scribble/dist/util/misc.js:16:11)
    at Object.interposeCall (/home/dimo/work/consensys/scribble/dist/instrumenter/interpose.js:245:16)
    at replaceExternalCallSites (/home/dimo/work/consensys/scribble/dist/instrumenter/instrument.js:406:45)
    at Object.instrumentContract (/home/dimo/work/consensys/scribble/dist/instrumenter/instrument.js:262:9)
    at instrumentFiles (/home/dimo/work/consensys/scribble/dist/bin/scribble.js:185:17)
    at Object.<anonymous> (/home/dimo/work/consensys/scribble/dist/bin/scribble.js:478:13)
    at Module._compile (internal/modules/cjs/loader.js:1137:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1157:10)
    at Module.load (internal/modules/cjs/loader.js:985:32)
    at Function.Module._load (internal/modules/cjs/loader.js:878:14)

We fail this assertion in interposeCall:

    if (call.vFunctionCallType === ExternalReferenceType.UserDefined) {
        const calleeDef = call.vReferencedDeclaration;

        assert(calleeDef !== undefined && calleeDef instanceof FunctionDefinition, ``);

        const fPtrT = makeFunPtrType(calleeDef, factory);

Esentially the logic here doesn't handle the case when calleeDef is a VariableDeclaration.

Handling it correctly here first requires a fix in solc-typed-ast to get the expected argument and return types of a getter for an arbitrary public state var.

cd1m0 commented 3 years ago

To give more context, the issue here arises because of the way we interpose on function calls. Specifically we generate a wrapper that takes a function pointer, and pass it a function pointer to the original fun. This requires getting the argument and return types of the original function, or in this case public variable def, which requires the fixes in solc-typed-ast to support getting the arg/return types of an arbitrary public var.

This approach has another problem however - we introduce function pointers during instrumentation for contracts that didn't have function pointers before. Function pointers at the moment are problematic for certain SMT based verification backends (SMTCheck and solc-verify) with which we would like to integrate. Furthermore, there is no fundamental reason why we need to use function pointers during instrumentation here. Instead we can specialize the wrappers to the receiving contract/address and just pass that in. This would makes us play nicer with smt verification backends.