Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No support for assembly staticcall #329

Closed
barakman opened this issue Jun 27, 2019 · 5 comments
Closed

No support for assembly staticcall #329

barakman opened this issue Jun 27, 2019 · 5 comments

Comments

@barakman
Copy link

barakman commented Jun 27, 2019

This utility in its current implementation fails upon testing a function which invokes any of the functions being tested (i.e., in any of the contracts being tested) via staticcall from an assembly block.

Here is the reason for this failure:

  • The tool adds an event before each line of code
  • Subsequently every constant function is changed to non-constant
  • The usage of staticcall, which was initially designated in order to invoke a constant function and retrieve its returned-value, fails silently (i.e., returns 0 without fetching the returned-value of the target function)
@cgewecke
Copy link
Member

cgewecke commented Jul 13, 2019

Cross referencing with openzeppelin-solidity 1761. It's possible this is fixed in solc 0.5.10.

@cgewecke
Copy link
Member

Oh actually it looks like it was fixed in the new vm

@cgewecke
Copy link
Member

Fixed in 0.6.2, see BancorProtocol PR ref above this comment

barakman added a commit to bancorprotocol/contracts-solidity that referenced this issue Jul 15, 2019
Subsequently, the following hack in `BancorNetwork.sol` is no longer needed:

    /* SC0 */ if iszero(staticcall(gas, _dest, add(data, 32), mload(data), ret, 64)) {revert(0, 0)}
    // SC1 // switch call(gas, _dest, 0, add(data, 32), mload(data), ret, 64) case 0 {revert(0, 0)}
    // Prior to being instrumented by solidity-coverage, the SC0 line should be disabled and the SC1 line should be enabled:
    // - The `if` should be replaced with a `switch` because it would otherwise yield a compilation error (sc-forks/solidity-coverage#328)
    // - The `staticcall` should be replaced with a `call` because it would otherwise yield a runtime error (sc-forks/solidity-coverage#329)
@barakman
Copy link
Author

barakman commented Jul 15, 2019

@cgewecke:
Thank you.

May I ask how it works?

I've checked the instrumented contracts, and nothing in the related aspect seems to have changed.

Here is the instrumented caller-function:

bytes4 private constant GET_RETURN_FUNC_SELECTOR = bytes4(uint256(keccak256("getReturn(address,address,uint256)") >> (256 - 4 * 8)));

function getReturn(address _dest, address _fromToken, address _toToken, uint256 _amount) internal returns (uint256, uint256) {
    emit __FunctionCoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',14);
    emit __CoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',428);
    emit __StatementCoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',60);
    uint256[2] memory ret;

    emit __CoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',429);
    emit __StatementCoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',61);
    bytes memory data = abi.encodeWithSelector(GET_RETURN_FUNC_SELECTOR, _fromToken, _toToken, _amount);

    emit __CoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',431);
    assembly {
        let success := staticcall(
            gas,           // gas remaining
            _dest,         // destination address
            add(data, 32), // input buffer (starts after the first 32 bytes in the `data` array)
            mload(data),   // input length (loaded from the first 32 bytes in the `data` array)
            ret,           // output buffer
            64             // output length
        )
        if iszero(success) {
            revert(0, 0)
        }
    }

    emit __CoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',445);
    emit __StatementCoverageBancorNetwork('C:/Users/.../BancorNetwork.sol',62);
    return (ret[0], ret[1]);
}

And here is the instrumented callee-function:

function getReturn(IERC20Token _fromToken, IERC20Token _toToken, uint256 _amount) public returns (uint256, uint256) {
    emit __FunctionCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',30);
    emit __CoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',503);
    emit __AssertPreCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',21);
    emit __StatementCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',68);
    require(_fromToken != _toToken);
    emit __AssertPostCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',21);

    // conversion between the token and one of its connectors
    emit __CoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',506);
    emit __StatementCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',69);
    if (_toToken == token) {
        emit __StatementCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',70);
        emit __BranchCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',22,0);
        emit __CoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',507);
        return getPurchaseReturn(_fromToken, _amount);
    }
    else {
        emit __StatementCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',71);
        emit __BranchCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',22,1);
        if (_fromToken == token) {
            emit __StatementCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',72);
            emit __BranchCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',23,0);
            emit __CoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',509);
            return getSaleReturn(_toToken, _amount);
        }
        else {
            emit __BranchCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',23,1);
        }
    }

    // conversion between 2 connectors
    emit __CoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',512);
    emit __StatementCoverageBancorConverter('C:/Users/.../converter/BancorConverter.sol',73);
    return getCrossConnectorReturn(_fromToken, _toToken, _amount);
}

As you can see, the callee-function is (no longer) constant, so I don't quite understand how staticcalling it from the caller-function completes successfully.

Thanks

@cgewecke
Copy link
Member

cgewecke commented Jul 15, 2019

@barakman Yes, it looks like this was fixed at ethereumjs-vm, specifically for the coverage client in this PR.

And that PR had small bug which we fixed here in our own vm.

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

No branches or pull requests

2 participants