Audit: MANETH-39

[sandybradley]

MEVETH OFT TRANSFERS MIGHT GET STUCK

SEVERITY: Medium

PATH: OFTCoreV2.sol:_sendAndCall:L179-205

https://github.com/manifoldfinance/mevETH2/blob/63edde66d91c263b919fe9c21e128a382219880e/src/layerZero/oft/OFTCoreV2.sol#L179-L205

REMEDIATION

the MevEth contract is only supposed to hold ETH, not its own shares, so we would recommend to implement an access-controlled recovery mechanism for tokens

DESCRIPTION

The contract MevEth implements OFTCoreV2 which allows for cross-chain OFT transfers. However, one transfer type might cause tokens to become stuck in the contract and MevEth doesn't have a recovery mechanism for tokens. It’s highly probable that a user might mix up packet type when sending OFT cross-chain. By sending PT_SEND_AND_CALL packet where the destination is an EOA (not a smart contract) instead of PT_SEND the tokens would get stuck in the contract.

In that scenario _sendAndCallAck is invoked from _nonblockingLzReceive:

if (packetType == PT_SEND) {
    _sendAck(_srcChainId, _srcAddress, _nonce, _payload);
} else if (packetType == PT_SEND_AND_CALL) {
>> _sendAndCallAck(_srcChainId, _srcAddress, _nonce, _payload);
} else {
    revert UnknownPacketType();
}

Further OFT amount is first transferred to MevEth:

// credit to this contract first, and then transfer to receiver only if callOnOFTReceived()
succeeds
if (!credited) {
>> amount = _creditTo(_srcChainId, address(this), amount);
    creditedPackets[_srcChainId][_srcAddress][_nonce] = true;
}

Because the destination is an EOA, OFT amount will stuck forever inside MevEth:

if (!_isContract(to)) {
    emit NonContractAddress(to);
    return;
}

https://github.com/manifoldfinance/mevETH2/blob/63edde66d91c263b919fe9c21e128a382219880e/src/layerZero/oft/OFTCoreV2.sol#L207-L247

[sandybradley]

I think sendAndCall can be safely removed, mitigating this issue.

[ControlCplusControlV]

Does that break compliance with the standard though? I am not familiar

[sandybradley]

No. OFT is not a standard, just the messaging protocol. All the layerzero tests and tasks only use the sendFrom function. If you look at 0xBeans version he also just uses the sendFrom function. https://github.com/manifoldfinance/depreciated-meveth/blob/c0c0847ca8a43d05d6180810d3067a83e1cb53c5/src/MevETH.sol#L138-L172

[ControlCplusControlV]

Oh ok, yeah let's just remove it then for 1) the codesize and 2) any sort of function which allows a default call is almost the culprit behind every single bug