ERC-20 deposit cctxs are refunded to the EOA instead of an intermediary contract

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/zetaclient/utils.go#L123

Vulnerability details

Impact

The EOA is set as the beneficiary of a failed and refunded inbound ERC-20 deposit cctx, instead of a potential intermediary contract. Funds can be stolen from this intermediary contract.

Additionally, but less severe, the zContext's origin is also set to the EOA.

Proof of Concept

Observers watch external EVM chains for the Deposited event emitted by the ERC20Custody.deposit function on ERC-20 token deposits. Specifically, these logs are processed by calling the GetInboundVoteMsgForDepositedEvent function in the following instances:

1. Observing the inbound transaction tracker, see node/zetaclient/inbound_tracker.go#L193
2. Querying the EVM chain, see node/zetaclient/evm_client.go#L894

However, in line 123, the MsgVoteOnObservedInboundTx message's Sender is set to the transaction signer EOA address (sender) instead of the caller (msg.sender) of the ERC20Custody.deposit function.

103: func (ob *EVMChainClient) GetInboundVoteMsgForDepositedEvent(event *erc20custody.ERC20CustodyDeposited) (types.MsgVoteOnObservedInboundTx, error) {
...   // [...]
115:    signer := ethtypes.NewLondonSigner(big.NewInt(ob.chain.ChainId))
116:    sender, err := signer.Sender(tx)
117:    if err != nil {
118:        ob.logger.ExternalChainWatcher.Error().Err(err).Msg(fmt.Sprintf("can't recover the sender from the tx hash: %s", event.Raw.TxHash.Hex()))
119:        return types.MsgVoteOnObservedInboundTx{}, errors.Wrap(err, fmt.Sprintf("can't recover the sender from the tx hash: %s", event.Raw.TxHash.Hex()))
120:
121:    }
122:    return *GetInBoundVoteMessage(
123: ❌      sender.Hex(),
124:        ob.chain.ChainId,
125:        "",
126:        clienttypes.BytesToEthHex(event.Recipient),
127:        common.ZetaChain().ChainId,
128:        sdkmath.NewUintFromBigInt(event.Amount),
129:        hex.EncodeToString(event.Message),
130:        event.Raw.TxHash.Hex(),
131:        event.Raw.BlockNumber,
132:        1_500_000,
133:        common.CoinType_ERC20,
134:        event.Asset.String(),
135:        ob.zetaClient.GetKeys().GetOperatorAddress().String(),
136:        event.Raw.Index,
137:    ), nil
138: }

It is very likely that an intermediary contract deployed on a supported EVM chain is interacting with the ERC20Custody contract. In this case, the EOA interacting with this contract and subsequently initiating the deposit of the ERC-20 tokens will be set as the Sender address.

If the cctx fails and a refund to the source chain is sent, this EOA will be the receiver of the funds. Depending on the business logic of the intermediary contract, this allows an EOA to steal its funds.

An inbound cctx can fail for various reasons, but most importantly, the error has to originate from node/x/fungible/keeper/deposits.go#L52-L85. For example, if the liquidity cap of the deposited asset is reached and the types.ErrForeignCoinCapReached error is returned, the cctx is considered failed and a refund is initiated.

Example

Given the following simple Solidity contract on Ethereum Mainnet that intends to keep user funds in escrow and some time later, allows the owner of the contract to bridge the aggregated escrowed funds to ZetaChain.

The owner calls the bridge function. The EOA address will be set as the Sender of the MsgVoteOnObservedInboundTx message.

If the inbound cctx fails and a refund is initiated, the EOA will receive all the funds. Consequently, the owner is able to steal the escrowed user funds from this contract

contract Escrow {
    ERC20Custody public custody;
    mapping (address => mapping (IERC20 => uint256)) public escrowed;
    address owner;

    modifier onlyOwner() {
        require(msg.sender == owner, "Only owner can call this function.");
        _;
    }

    constructor(ERC20Custody custody_, address owner_) {
        custody = custody_;
        owner = owner_;
    }

    function escrow(IERC20 asset, uint256 amount) {
        asset.transferFrom(msg.sender, address(this), amount);

        escrowed[msg.sender][asset] += amount;
    }

    function bridge(
        bytes calldata recipient,
        IERC20 asset,
        uint256 amount,
        bytes calldata message
    ) external onlyOwner {
        asset.approve(address(custody), amount);
        custody.deposit(recipient, asset, amount, message);
    }
}

Tools Used

Manual review

Recommended mitigation steps

Consider adding the msg.sender of the ERC20Custody.deposit function as an additional event parameter to the Deposited event. This parameter can then be used to set the Sender of the MsgVoteOnObservedInboundTx message.

Assessed type

Other

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-sponsor]

lumtis (sponsor) confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report