Fix (audit AUR-03): signer_id for: deposit and withdraw
#14
Conversation
There was a problem hiding this comment.
General design question: is this the right fix for this issue? An alternative fix would be to remove relayer_id entirely (or always set it to some null value if we care about backwards compatibility). If we are removing the fee logic, and this is the only place the relayer_id gets used then I think it is better remove relayer_id also.
But the second point is related to |
|
No, owner can be something else like a DAO, or a "trusted account" but we don't want to do that. Currently, it is the contract itself, same with the Aurora EVM. |
We could do the same check we do for other admin-controlled functions (using the predecessor account id directly). Why should we force these allowed accounts to go through the Aurora Engine to access the NEP-141 contract? |
| Ok(res) | ||
| } | ||
|
|
||
| pub async fn call_deposit_eth_to_aurora(&self) -> anyhow::Result<()> { | ||
| let proof: Proof = serde_json::from_str(PROOF_DATA_ETH).unwrap(); | ||
| let res = self.deposit_with_proof(&proof).await?; | ||
| assert!(res.is_success()); | ||
| assert!(res.is_success(), "call_deposit_eth_to_aurora: {:#?}", res); |
There was a problem hiding this comment.
Just a nit, asserts or panic messages should be formatted as full sentences with a capital as it makes grammatical sense on the panics in the return in the terminal.
Description
After Sigma Prime Aurora Engine and Eth-Connector audit, was detected potential issue: AUR-03 - Incorrect Use of Predecessor Account.
Details
The functions
deposit()andwithdraw()as well as theNEP-141logic in aurora-eth-connector make use of thepredecessor_account_id()function. Since thepredecessor_account_id()will always be "aurora" (i.e. engine contract), some code functionality is broken.The updates to
aurora-eth-connectorremove logic from the engine contract. The new logic is inserted into a new contract called aurora-eth-connector.Functions in
aurora-eth-connectorsuch asdeposit(), withdraw() and theNEP-141logic are only called from the engine contract. Therefore, in these functions,predecessor_account_id()will always be the engine contract.Within
deposit(),predecessor_account_id()is used to determine ownership and set therelayer_id. Each of the use cases are invalid when the predecessor is always the engine contract.Similarly, the
withdraw()function also uses thepredecessor_account_id()to determine ownership.Gas cost
Not changed.
Solution
Added methods
is_ownerto manage access rights when paused. If it'sis_ownerthen account has access right with modification operation to paused methods.How to review
Pay attention to methods:
deposit- owner check for admin controlwithdraw- owner check for admin controlTests
Tests were extended with new functionality for
is_owner.