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!