Adding "x" as reason in expectRevert causes test to pass

[alsco77]

Description

Specifying "x" as the 'expectedError' in the 'expectRevert' call causes the test to pass, even when "x" does not appear in the parsed error message.

This might not be directly a bug, although I can't see why it should be getting through the following check in expectRevert.js#9 -> if (error.message.indexOf(expectedError) === -1) {.

Either way it should certainly be more strict in this instance.

[nventuro]

Hello @alsco77, thanks for reporting this! Could you provide a contract and a test snippet so we can reproduce this issue to better understand it?

[alsco77]

Sure @nventuro - in the below test file, both tests pass.

SafeToken.sol

pragma solidity 0.5.16;

import { SafeERC20, IERC20 } from "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";

contract SafeToken {

    using SafeERC20 for IERC20;

    IERC20 public token;

    constructor(IERC20 _token) public {
        token = _token;
    }

    function sendToken(address recipient, uint256 amt)
        external
    {
        token.safeTransferFrom(msg.sender, recipient, amt);
    }
}

Test.spec.ts

import { expectRevert } from "@openzeppelin/test-helpers";

const SafeToken = artifacts.require("SafeToken");
const MockERC20 = artifacts.require("MockERC20");

contract("Test", async (accounts) => {
    let safeToken;

    beforeEach(async () => {
        const token = await MockERC20.new("MOCK", "MK1", 18, accounts[0], 10000);
        safeToken = await SafeToken.new(token.address);
    });

    it("should fail because x is not the expectedReason", async () => {
        await expectRevert(safeToken.sendToken(accounts[0], 1, { from: accounts[1] }), "x");
    });
    it("should pass because the expectedReason is correct", async () => {
        await expectRevert(
            safeToken.sendToken(accounts[0], 1, { from: accounts[1] }),
            "SafeERC20: low-level call failed",
        );
    });
});

  Contract: Test
    ✓ should fail because x is not the expectedReason (26826 gas)
    ✓ should pass because the expectedReason is correct (26826 gas)

[abcoathup]

Hi @alsco77,

I was able to reproduce using OpenZeppelin Test Environment as the test runner.

A.sol

// contracts/A.sol
// SPDX-License-Identifier: MIT
pragma solidity ^0.6.0;

contract A {
    function alwaysRevert() public {
        revert("A: reason");
    }
}

A.test.js

// test/A.test.js

const { contract } = require('@openzeppelin/test-environment');

// Import utilities from Test Helpers
const { expectRevert } = require('@openzeppelin/test-helpers');

const A = contract.fromArtifact('A');

describe('A', function () {
  beforeEach(async function () {
    this.contract = await A.new();
  });

  it('should fail as reason is not x', async function () {
    await expectRevert(this.contract.alwaysRevert(), 'x');
  });
  it('should pass', async function () {
    await expectRevert(this.contract.alwaysRevert(), 'A: reason');
  });
});

Test

$ npm run test

> issue125@1.0.0 test /home/abcoathup/projects/forum/issue125
> mocha --exit --recursive test



  A
    ✓ should fail as reason is not x (96ms)
    ✓ should pass (45ms)


  2 passing (571ms)

[nventuro]

The test is passing because the complete error message includes an 'x':

Returned error: VM Exception while processing transaction: revert {message}

We're currently matching against the entire string, not just the revert reason. I think I recall @frangio at one point considering stripping the message at the beginning, we may have decided not to do so due to there being no guarantees of its contents accross versions and clients. Considering the helper is designed for you to input the entire expected revert reason, I think this makes sense.

[frangio]

Yes it's kind of hard to compare against the entire message because of incompatibilities between clients... But we could try it out and see how it works.

A middle point would be to require the message to match at the end of the string, rather than at any point.

[alsco77]

Yeah I think that there is some middle ground to stand on. I understand that the whole string is being matched against, but passing with 'x' seems like its being much too liberal, especially given that that part of the message is generally disregarded. Requiring a larger portion of the string to be matched, whether that is at the end or simply a full word somewhere seems like a fair patch to me.

[nventuro]

An issue with matching at the end is that we'd be forcing users to input the entire revert reason, which is not a requirement today. A best effort approach to remove the leading message ('vm exception') might be less disruptive.

[csokun]

In my first attempt, I was expecting an exact reason matching excluding the generic vm exception ... prefix of cause. But I was wrong so after reviewing the implementation then I realized expectRevert is a partial match.