Forking mainnet results in erroneous revert reason

[mds1]

This issue has been noticed by the Compound team before when forking the mainnet, and can be reproduced using the tests in this commit.

Steps to Reproduce

1. Clone the above repository at the specified commit
2. Run npm install
3. Compile contracts and run tests with npx oz compile && npx mocha --exit --timeout 0 test/endaoment-test.js
4. The tests will fail with the following console output

$ npx oz compile && npx mocha --exit --timeout 0 test/endaoment-test.js
Nothing to compile, all contracts are up to date.


  Endaoment
    ✓ should see the deployed Endaoment contract
    1) should allow a membership proposal & vote


  1 passing (39s)
  1 failing

  1) Endaoment
       should allow a membership proposal & vote:
     Error: Returned error: VM Exception while processing transaction: revert Endaoment::processProposal - token transfer to guild bank failed -- Reason given: re-entered.
     at PromiEvent (node_modules/@truffle/contract/lib/promievent.js:9:30)
      at TruffleContract.processProposal (node_modules/@truffle/contract/lib/execute.js:169:26)
      at Context.it (test/endaoment-test.js:61:29)
      at process._tickCallback (internal/process/next_tick.js:68:7)

Explanation

In the file endaoment-test.js we are failing in the test called "should allow a membership proposal & vote" at the line await this.instance.processProposal(0, {from: summoner});

This line calls the function processProposal() in Endaoment.sol. The failure occurs in the following section of that function:

// transfer tokens to guild bank
require(
    guildBank.deposit(proposal.tokenTribute),
    "Endaoment::processProposal - token transfer to guild bank failed"
);

This line is calling the deposit() function in GuildBank.sol. This function is shown below for convenience:

function deposit(uint256 amount) public onlyOwner returns (bool) {
    bool transferSuccess = approvedToken.transferFrom(msg.sender, address(this), amount);
    if (!transferSuccess) {
        return false;
    }
    
    uint256 mintCode = cToken.mint(amount);
    if (0 == mintCode) {
        return false;
    }
    emit Deposit(amount);
    return true;
}

Now, the statement if (0 == mintCode) is incorrect, and really should be if (0 != mintCode). However, this mistake in the code means the function should return false and therefore the contract should error solely with the error message "Endaoment::processProposal - token transfer to guild bank failed".

Instead, you can see the cToken error message of "re-entered" is included. This is from the nonReentrant modifier of cTokens, but because there is no reentrancy here this should not have been triggered.

[jflatow]

Yes we have been experiencing this a lot recently (on both mainnet and ropsten now). We had seen similar issues before with delete wiping the fork state, leading to refetching the data in future calls instead of returning 0. We patched this here: compound-finance#5, but figured this was probably inappropriate as a general solution.

Reproducing the issue for us generally involves our entire stack, which is why we haven't reported thus far - now here is a nice self-contained instance of the issue, hoping yall can help us track down what is wrong - happy to help if we can.

[jflatow]

Note that this issue is popping up again, even with our patched version, so there seems to be another related issue.

[jflatow]

OK I take it back, after further investigation the issue is not popping up using the patched version (compound-finance#5), it was using a different version.

I've added a test to that PR to help reproduce the issue (and shows that the patch fixes it) in case someone attempts an 'ultimate fix', but at least if anyone else is blocked by this issue, they can use our branch for now.

[BlinkyStitt]

I think I'm seeing this issue in my tests too. I'm getting a revert on re-entrancy in one of my tests where that doesn't make sense. Should I provide it as an example?

[datgrog]

Same here, thanks @jflatow for some improvements over this, but I still get it :/

[jflatow]

Same here, thanks @jflatow for some improvements over this, but I still get it :/

You sure you are using the fork? We don't have the problem anymore using it, but if you can you should try to give a repro, in case there's another issue.

[datgrog]

Edit: Sorry, wrong ganache-cli ... 😅Thanks again for your patch

[davidlucid]

Does anyone know when this patch will be merged?

[mikeseese]

Hey @davidlucid I'm looking into the issue now and proposed patch from compound-finance#5, so hopefully soon!

[mikeseese]

As an FYI to this thread, the proposed fix in compound-finance#5 was non-generic and would cause other issues. #612 modifies the usage of _deleted to solve the issue

[mikeseese]

This was just officially released in [email protected] / [email protected]

[mds1]

I'm using ganache-core 2.11.3-beta.0, and it seems I'm still seeing this bug in certain cases.

Steps to reproduce:

1. Clone this repo at the linked commit
2. Install packages with yarn
3. Create file called .env with INFURA_ID=yourInfuraId
4. Run tests with yarn test

In step 4, only two tests in batch-zkSync-deposit-test.js will run. The first will pass, and the second will fail with Error: Returned error: VM Exception while processing transaction: revert ReentrancyGuard: reentrant call. We can verify there is no reentry as follows:

1. Modify line 116 to it.skip to skip that test. Run tests again, and our previously failing test now passes
2. Undo the previous step. Now switch the order of these two tests (line 116 and line 125). The second test fails with the same error, even though both tests pass when run individually. So something is odd with the ordering of the tests here

The reentrant call error is from the zkSync contract, found here.

The full console output from the error is below:

Error: Returned error: VM Exception while processing transaction: revert ReentrancyGuard: reentrant call -- Reason given: ReentrancyGuard: reentrant call.
     at PromiEvent (node_modules/@truffle/contract/lib/promievent.js:9:30)
      at TruffleContract.deposit (node_modules/@truffle/contract/lib/execute.js:169:26)
      at Context.<anonymous> (test/batch-zkSync-deposit-test.js:124:46)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

It seems this has something to do with the use of evm_revert. In the beforeEach and afterEach hooks we save and restore the ganache state using evm_snapshot and evm_revert. If you comment out the evm_rever functionality (89 and 102) the above error does not happen.

I also just noticed I'm missing some awaits in the beforeEach hook, but adding those in does not resolve this issue

[mikeseese]

@mds1, apologize for the delay. I had to focus on some Truffle Teams issues.

I can reproduce your issue! Thank you for the detailed write up and reproduction steps. I'm going to continue discussing this on #611 as I'm fairly certain they're the same issue. Looking at your original issue report here, this is also likely the same issue you were getting when you first made this issue. I'll keep this one closed as we did solve a different use cause for a similar symptom with the Compound-proposed fixes

[mrice32]

@seesemichaelj I know this issue is closed, but we've had a very similar re-entrancy guard issue with ganache forking. This update fixed our issue in release 6.10.2. But after trying different versions, we noticed a regression in versions 6.12.0+.

You can repro the issue in this repo: https://github.com/UMAprotocol/launch-emp by tweaking the version of ganache-cli in the package.json. We've found we get re-entrancy breakages in 6.10.1, 6.12.0, 6.12.1. We find no issue in 6.10.2 and 6.11.0.

Once you have pulled the repo and tweaked the version:

yarn
yarn ganache-cli --fork YOUR_INFURA_NODE -l 12000000

In a different shell:

node index.js --gasprice 100

[mikeseese]

Unfortunately @mrice32, I'm no longer with truffle and won't be able to help you out. @davidmurdoch is the point person for all ganache issues going forward. Best of luck!

[mrice32]

@seesemichaelj ahh, good to know. Thanks for the quick reply! @davidmurdoch, would be great to get your insight here!

[0xTimepunk]

I am having this issue too with the latest ganache version. Anyone?