pendulum-chain / vortex

https://app.vortexfinance.co/
1 stars 1 forks source link

Split our receiver contract on Moonbeam in two parts #137

Closed TorstenStueber closed 2 months ago

TorstenStueber commented 2 months ago

While resolving the out of gas problem on Moonbeam, we need to implement a workaround.

The idea of this workaround is to split the action that our receiver contract (0x066d12e8f155c87a87d9db96eac0594e872c16b2) executes into two parts:

TODO

gianfra-t commented 2 months ago

I'm still trying to digest the system but so far it looks solid. A few questions maybe you thought through more:

In any case, these front run call will fail since the approved balance of our contract would not match, as I imagine the attacker's intentions is to also not approve the corresponding balance. Since transactions are atomic as we discussed when investigating Axelar, if squid router calls arrives first and approves, then it must be that the initXCM is also called by them with the proper payload.

TorstenStueber commented 2 months ago

You are right about the approved balance. I confused how the funds arrive in the receiver account. This simplifies the security. I changed the ticket accordingly.

We could protect from all this by simply restricting the call to initXCM to the particular squid router contract. Then only thing an attacker could do is initiate the execution for us.

I thought about hard coding the address but then I thought that there is no guarantee about what relayer is going to execute the call (it can be any Axelar relayer and we don't know them). However, we already use a hardcoded address to transfer the funds from (is this actually a good idea or could that change? Does Axelar make guarantees here?)

About gas estimation, do you think this post hook would be estimated correctly? Because otherwise we may end up in the same situation as before if we ran out of gas for this initXCM.

I think the chance is high that it works correctly as it is only a few simple EVM operations. The main issue with the current situation is that execution leaves EVM and transfers to Substrate through the precompiled contract.

gianfra-t commented 2 months ago

However, we already use a hardcoded address to transfer the funds from (is this actually a good idea or could that change? Does Axelar make guarantees here?)

I do not understand enough the protocol, but I would assume that it is okay to trust the relay contract, otherwise any token transfer would also be insecure.

TorstenStueber commented 2 months ago

After investigating @gianfra-t's split contract, it turned out that indeed it was running out of gas, however, not out of EVM gas but out of gas of an additional alternative metering system that Moonbeam uses to measure POV size. I will explain more on the Moonbeam issue later.

For now I was testing a different idea that I pushed to the branch. See splitReceiver2.sol. I deployed that contract to 0x388b695805aad4dca5f8504cc40932d57f8f2aa0 and verified and submitted the code source to Moonscan so that it is also available in Tenderly.

With this alternative approach we store only the keccac256 hash of (id, amount, payload) and the mapping in the contract just maps hashes to bool (so it behaves like a set). This way we stay under the storage limit.

In the second call to executeXCM we would then need to provide all values (id, amount, payload) and the contract just checks whether the hash is in the map. If it is, then it proceeds and executes the X-Tokens call and removes the hash from the set.

I didn't test that part yet but I think that the idea works and is secure. At least the initXCM call worked without problems with Squid router.

TorstenStueber commented 2 months ago

I changed the description in order to reflect the version we are implementing right now.

prayagd commented 2 months ago

Can we add a rough estimate to this ticket?