ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
22.76k stars 5.64k forks source link

Called contract that reverts does not revert whole transaction #13983

Closed migoldfinger closed 1 year ago

migoldfinger commented 1 year ago

Description

First, I am not sure if this is a bug or just an misunderstanding on my side. I created a proxy contract that calls multiple other contracts multiple times. The intention was that if one call does not end successfully the hole transaction should be reverted. The method looks like this:

function proofOfConcept(MetaPath[] calldata metaPath, uint256 amount) override external returns (uint256)
{
    uint256 startBalance = amount;
    uint256 intermediateTokenBalance = amount;
    uint256 len = metaPath.length;

    unchecked
    {
        for(uint256 i = 0; i < len; ++i)
        {
            MetaPath memory p = metaPath[i];
            IERC20(p.path[0]).approve(p.subcontract, intermediateTokenBalance);
            uint256[] memory amountOuts = ISubContract(p.subcontract).trade(
                intermediateTokenBalance,
                p.path,
                address(this),
                block.timestamp + 300 // solhint-disable-line not-rely-on-time
            );
            intermediateTokenBalance = amountOuts[amountOuts.length - 1];
        }
    }
    require(intermediateTokenBalance > startBalance, "Trade Reverted");
    return intermediateTokenBalance;
}

I did read the documentation and got the information that calls to other contract especially if they are made with the interface should be handled correctly and if they fail the transaction should be reverted. However I am seeing in my tests that if ISubContract(p.subcontract).trade(IntermediateTokenBalance); is called successfully and a following call reverts I end up with whatever token the successfully call returns stored in my proxy contract. The call of the proxy contract even returns with an error but not the whole transaction is reverted.

Environment

Steps to Reproduce

Steps to reproduce is to call the proxy contract multiple times till the error appears. I did not notice it for a few hours in my long running test. At first I did notice a drain of my test tokens I could not explain and then found other tokens hold by my proxy contract that should not be there at all. I did than track the culprit down to the method posted above.

An easier way reproduce would be using 2 Subcontracts one that succeeds and one that fails ever time and call that, no need for an loop and bad luck for the error to appear.

I see 2 options Option 1, this should not happen and the transaction as a whole should be reverted. Option 2, I did miss something, in that case I would love to learn of a method to revert the whole transaction when a call to a subcontract fails even if a previous call succeeds.

migoldfinger commented 1 year ago

I tried to reproduce the case with an test that forces that behavior. I was not able to reproduce it. What I found is that one of the called contracts did in fact not revert the transaction on failure and instead returns an special value to signal that there was a failure. Unfortunately the called contract is not in my domain so I ended up with a special handling for this single contract.

So I can now tell that failing subcontracts do in fact react as expected (if they are properly coded)