Improper condition for message validation check in deposit flow

[birchmd]

At the time of writing, the following validation logic exists in the ft_transfer_call implementation:

https://github.com/aurora-is-near/aurora-eth-connector/blob/ded0cfd6735bdf4a0371a811b5ed7731bdaa072c/eth-connector/src/lib.rs#L175-L183

As I understand it, the purpose of this validation is to prevent sending incorrect data to Aurora Engine's ft_on_transfer method in the case that a call to this contract's deposit method is supposed to deliver the ETH to an Aurora account. In that is true then the condition where the validation happens should be

if receiver_id == self.engine_account_id 

where engine_account_id will need to be a new config field in this contract.

I think the reason for this mistake in the code is that when the Connector and Engine were one contract then the sender_id == receiver_id did work since it was true that connector_account_id == sender_account_id == engine_account_id.

That said, in a world where there is more than one instance of the Aurora Engine contract, and they all share this connector as a source of ETH, then it might be useful to perform this check in case the receiver_id is from a set of known engine accounts. For example, it might look something like

if self.known_engine_accounts.contains(&receiver_id)

[mrLSD]

@birchmd I found out why this collision exists in this part of the code.

Reference implementation for ft_transfer_callassumes, that ft_on_transfer contract call should be called sender_id, where

        let sender_id = env::predecessor_account_id();

but in Engine it is:

        let ft_on_transfer_call = PromiseCreateArgs {
            target_account_id: receiver_id,
            method: "ft_on_transfer".to_string(),
            args: data1.into_bytes(),
            attached_balance: ZERO_ATTACHED_BALANCE,
            attached_gas: prepaid_gas - GAS_FOR_FT_TRANSFER_CALL - GAS_FOR_RESOLVE_TRANSFER,
        };

[mrLSD]

@birchmd Considering the above - your remark is the place to be. However, with an important note - this is very different from the reference implementation.

where engine_account_id will need to be a new config field in this contract.

Do you mean implement additional functions like:

• set_known_engine_account
• get_known_engine_accounts
• delete_known_engine_account

As for me, it makes more sense to define them as a constant.

[birchmd]

Yes, I was imagining having those various maintenance functions for controlling the known engine contracts. This would let us dynamically keep the connector up to date with new instances of the Aurora Engine. But I understand this also adds complexity to the contract, so it is maybe not worth it and a constant would be better as you suggested. What do you think @joshuajbouw ?

Another option is to remove that validation logic entirely, and avoid this problem altogether. In the case that the fee is too large it means the deposit flow will fail to mint tokens to the user on Aurora. Instead the tokens will be left minted with the connector account, which means manual intervention could fix the issue (move the tokens to the right account). But this case should never come up regardless because the rainbow bridge does not use the fee mechanism anyway as far as I remember.

[joshuajbouw]

the rainbow bridge does not use the fee mechanism anyway as far as I remember.

Thats correct. It technically "does" exist, but was not actually used, and we had to even disable this in the Engine to set it to 0 else relayer could steal. Then simply could just remove this entirely. Would need to triple check with @sept-en though.

This would let us dynamically keep the connector up to date with new instances of the Aurora Engine.

Still current running idea is only bridging to Aurora EVM only.

A constant is not ok because if we were to deploy this locally for testing you can not update the contract unless you do it in the code itself. Therefore, the best path forward would be to add in the contract address in the config as a new field.

[mrLSD]

@birchmd I don't think, we can remove it.

I just wanted to remind you, that part of the code checks message correctness. And it's not only about fees.
It is also for:

self.mint_eth_on_aurora(
    message_data.recipient,
    Wei::new(U256::from(args.amount.as_u128())),
)

If after the message verifies in the ft_transfer_call we fail, then the transaction will fail. But if it will fail in ft_on_transfer transaction pass successfully, but the receipt fails. We definitely need message correctness check before sending it to ft_on_transfer.

[mrLSD]

After discussion with @joshuajbouw , the conclusion is:

1. Check engine accounts from the config field from storage, like this:

if self.known_engine_accounts.contains(&receiver_id) {
     let _ = FtTransferMessageData::parse_on_transfer_message(&msg).sdk_unwrap(); 
}

2. Do not check amount of fee.

3. Add new funcions:

• set_engine_account
• get_engine_accounts

[mrLSD]

Fixed in #12