Reentrancy Attack on mimicCall Function

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/MsgValueSimulator.sol#L62

Vulnerability details

Impact

Could be used by an attacker to carry out a reentrancy attack on the contract, the attacker might then be able to make several calls to the contract in a single transaction, which could lead to the loss of money or undesired behavior.

Proof of Concept

Let's assume the following situation to illustrate the reentrancy vulnerability:

• There is a contract MaliciousContract that has a fallback function.
• MsgValueSimulator contract is calling MaliciousContract in its fallback function through mimicCall`.
• MaliciousContract calls back to MsgValueSimulator in its fallback function, before the mimicCall function finishes executing.

Illustration of how the reentrancy vulnerability is exploited by the MaliciousContract contract:

contract MaliciousContract {
    address private _msgValueSimulator;

    constructor(address msgValueSimulator) {
        _msgValueSimulator = msgValueSimulator;
    }

    fallback() external payable {
        // Reentrancy attack
        (bool success, ) = _msgValueSimulator.call(abi.encodeWithSignature("fallback(bytes)", msg.data));
        require(success);
    }
}

In this case, the fallback function of MaliciousContract calls the fallback function of _msgValueSimulator, which can then call back MaliciousContract before it updates its balance1.

In the constructor of MaliciousContract, i store the address of MsgValueSimulator, In the fallback function of MaliciousContract, then callMsgValueSimulatorusing the stored address and pass along themsg.data. This causesMsgValueSimulatorto call back toMaliciousContract, which in turn calls back toMsgValueSimulator`, and so on, creating an infinite loop.

Tools Used

Recommended Mitigation Steps

The checks-effects-interactions design pattern, where checks come first, then effects, and then interactions, can be used with MsgValueSimulator, In this instance, the interaction is the mimicCall, and the results are the transfer of ETH and changing the msg.value for the subsequent call. We can guarantee that the state is not changed if the checks are unsuccessful by conducting the checks before to the effects, thwarting the reentrancy attack.

Implementing this pattern.

fallback(bytes calldata _data) external payable onlySystemCall returns (bytes memory) {
    (uint256 value, bool isSystemCall, address to) = _getAbiParams();

    // Check if the contract has already been called
    require(!_currentlyExecuting);

    // Set the flag to true to prevent reentrancy attack
    _currentlyExecuting = true;

    if (value != 0) {
        (bool success, ) = address(ETH_TOKEN_SYSTEM_CONTRACT).call(
            abi.encodeCall(ETH_TOKEN_SYSTEM_CONTRACT.transferFromTo, (msg.sender, to, value))
        );

        // If the transfer of ETH fails, we do the most Ethereum-like behaviour in such situation: revert(0,0)
        if (!success) {
            assembly {
                revert(0, 0)
            }
        }
    }

    if (value > MAX_MSG_VALUE) {
        assembly {
            return(0, 0)
        }
    }

    SystemContractHelper.setValueForNextFarCall(uint128(value));

    // Reset the flag to false after the effects have been performed
    _currentlyExecuting = false;

    return EfficientCall.mimicCall(gasleft(), to, _data, msg.sender, false, isSystemCall);
}

A "boolean" variable called _currentlyExecuting has been created and is set to true at the beginning of the function and to false when the effects have been seen. Before calling the method, I've included a check to make sure _currentlyExecuting is false, because any further calls to the function while _currentlyExecuting is true would fail the check and roll back the transaction, this disables the reentrancy attack.

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[GalloDaSballo]

It will not call the recipient meaning you cannot re-enter