Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split our receiver contract on Moonbeam in two parts #137

Closed
TorstenStueber opened this issue Sep 5, 2024 · 6 comments · Fixed by #139
Closed

Split our receiver contract on Moonbeam in two parts #137

TorstenStueber opened this issue Sep 5, 2024 · 6 comments · Fixed by #139
Assignees
Labels
priority:high Do it now

Comments

@TorstenStueber
Copy link
Member

TorstenStueber commented Sep 5, 2024

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:

  • record every call to it (from squid router) into an internal mapping
  • add another function that allows to continue a call and that will actually call the xtokens contract

TODO

  • generate a random id for every flow
  • before creating the Squid router transactions generate a hash as follows
    • SHA256 of the Solidity packed encoding of (id, payload), where payload is the argument of the executeXCM function of the current receiver contract on Moonbeam
  • store the id and hash in the local storage state (offrampingState) and also record it in the Google sheet dump
  • add a public mapping to the receiver contract from hash to amounts (uint256)
  • add a new function initXCM to the receiver contract that takes a hash and an amount argument
    • reverts if amount is 0
    • checks whether there is already an entry in the mapping under the hash, if yes, then revert
    • executes the transferApprovedTokensToSelf call
    • adds entry (hash -> amount) to the mapping
  • change the squid router post hook call into our receiver contract
    • call initXCM instead of executeXCM
    • provide the hash and the amount (just 0 at this point as it will be correctly replaced by the squid router contract later)
  • change the executeXCM call so that it takes an id and a payload
    • determines the hash from the id and payload as above (SHA256 of the packed encoding)
    • reverts if there is no non-zero entry under hash in the mapping
    • looks up the amount from the mapping and executes the xtokens call
    • removes the respective entry from the mapping
  • create a new Moonbeam account ("moonbeam executor") that we fund with some GLMR
  • the secret of that account will be provided to the signer service through a secret environment variable MOONBEAM_EXECUTOR_PRIVATE_KEY
  • add an endpoint to the backend that takes an id and payload
    • that backend will call the executeXCM function of the receiver contract and uses the moonbeam executor account for that call
  • add a new phase to the offramping flow, executeXCM
    • that phase is between pendulumFundEphemeral and subsidizePreSwap
    • it will merely call the new backend endpoint and then wait until the XCM is completed
  • change the pendulumFundEphemeral phase so that it only waits until there is an entry in the new receiver contract mapping for the respective hash
@gianfra-t
Copy link
Contributor

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

  • The only security concern about an attacker front running with an identical id is about blocking the operation, but not really a risk of stealing.

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.

  • 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.

  • 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.

@prayagd prayagd added the priority:high Do it now label Sep 5, 2024
@TorstenStueber
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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

@prayagd
Copy link
Collaborator

prayagd commented Sep 9, 2024

Can we add a rough estimate to this ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:high Do it now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants