Split NEP-141 logic outside of Engine #5

mrLSD · 2022-07-28T11:27:59Z

At the moment, the NEP-141 logic is one of the main parts of the Aurora
contract. NEP-141 - implements the fungible token logic, which is the
link between the Ethereum and NEAR ecosystem tokens.

Due to the specifics of the original goals of the Aurora contract,
the logic of NEP-141 has become an integral part of the core contract itself.

However, further use and development of Aurora showed that this solution
has a number of shortcomings, which are proposed to be solved in this
proposal.

In the most general form, these include:

improved security (due to a number of vulnerabilities found recently)
more flexible changes (this will not require updating the entire aurora contract)

The result of the split should be a separate contract that the Aurora
contract interacts with through cross-contract calls.

Forum discussion: https://forum.aurora.dev/t/split-nep-141-logic-outside-of-engine/368

AIPs/aip-split_nep141_logic_outside-of_engine

birchmd · 2022-08-02T13:23:35Z

AIPs/aip-split_nep141_logic_outside-of_engine

+## Security Considerations
+
+One of the reasons for the emergence of AIP is security issues. 
+A number of vulnerabilities that were previously discovered separate 


Links to the issues would be good. Also describing why having the logic in a separate contract would have avoided those issues is important so that people understand why this proposal is related to those issues.

I will be grateful for links to Github Issues and PRs that relate to vulnerabilities found and fixed earlier.

birchmd · 2022-08-02T13:31:08Z

AIPs/aip-split_nep141_logic_outside-of_engine

+* new contract codebase - the current codebase that has been polished 
+for 1.5 years is stable and tested. The new contract code includes the 
+risks of finding bugs or vulnerabilities


I think there should be a separate drawbacks section for the proposal as a whole. In that section we can address those drawbacks to describe why we think we should go forward with the proposal in spite of the drawback.

In this particular case, I think saying the current code has been tested for 1.5 years is significantly weakened by the fact that the code has has multiple high-impact vulnerabilities in the past few months. Obviously it means that no one was looking critically enough at the code until there was a bug bounty to incentivize people to do so. That said, it is of course still true that changing code (including moving it elsewhere) always has the opportunity of introducing new bugs, so care is needed. However, the reason we want to make this change in the first place is because it makes whole classes of bugs impossible (namely those related to having arbitrary computation, ie the EVM, co-located with this sensitive contract), so it should still be true that the final result is more secure than things are currently.

AIPs/aip-split_nep141_logic_outside-of_engine

birchmd · 2022-08-02T13:35:59Z

AIPs/aip-split_nep141_logic_outside-of_engine

+One of the side effects of moving the NEP-141 logic into a separate 
+contract could be an increase in the cost of Gas. This is due to 
+cross-contract calls, as well as using the `near-sdk` implementation. 
+Previously, the `no-std` implementation was used, taking into account 
+the reduced gas cost. However, these costs are expected to be 
+negligible compared to the benefits.


How much will the increase be? What percentage of historical transactions would fail after this change?

We need to back up our arguments with data instead of unjustified claims.

How much will the increase be? What percentage of historical transactions would fail after this change?

We need to back up our arguments with data instead of unjustified claims.

This statement refers to a hypothesis, and cannot be confirmed before the implementation of the contract. However, it should be emphasized and identified as a possible problem - pitfalls. And this may well be. Therefore, the emphasis is on this.

I think it is on us to not simply provide hypotheses, but back them up with data. For example, we could implement a POC and measure the gas differences.

Take a look at EIP-2565 for example, it shows plots of how the gas and time correlations change with that proposal. In our case we probably don't need any plots, but still providing concrete data I think is important.

In our case, gas data can be estimated after implementation.

birchmd · 2022-08-02T13:37:18Z

AIPs/aip-split_nep141_logic_outside-of_engine

+[TODO: need verification] Potentially there is a risk that, due to the 
+specifics of the cross-contract call, the recipe may fall into the 
+next block, which will increase the overall execution of the request. 
+In turn, this can lead to security issues that were previously not 
+specific to the logic of interaction with NEP-141.


It is true that adding an extra cross-contract call will increase the time by 1 block (1 second). It's unclear to me what the security impact of that would be though.

We will certainly have 1 block time added, but we can mitigate this by enabling contract access to certain methods for this contract.

AIPs/aip-split_nep141_logic_outside-of_engine

Grammar changes Co-authored-by: Michael Birch <birchmd8@gmail.com>

joshuajbouw

In the first review, besides Michael's comments he made already, the overall wording is not quite clear to the audience.

There are problems with conciseness and clarity throughout the document, which may lead to an unclear understanding as a whole for the audience.

AIPs/aip-split_nep141_logic_outside-of_engine

joshuajbouw · 2022-08-03T19:25:33Z

AIPs/aip-split_nep141_logic_outside-of_engine

+[TODO: need verification] Potentially there is a risk that, due to the 
+specifics of the cross-contract call, the recipe may fall into the 
+next block, which will increase the overall execution of the request. 
+In turn, this can lead to security issues that were previously not 
+specific to the logic of interaction with NEP-141.


We will certainly have 1 block time added, but we can mitigate this by enabling contract access to certain methods for this contract.

AIPs/aip-split_nep141_logic_outside-of_engine

olga24912 · 2023-05-02T10:41:14Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+link between the NEAR NEP-141 tokens and Aurora ERC-20 compatible tokens.
+
+Due to the specifics of the original goals of the Aurora contract, 
+the logic of NEP-141 has become an integral part of the core contract itself.


What are the original goals of the Aurora contract? Do you have any links with these goals' descriptions? Original goals are relevant now, or have they changed somehow?

olga24912 · 2023-05-02T11:06:33Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+
+Currently, the NEP-141 logic is one of the main parts of the Aurora 
+contract. NEP-141 - implements the fungible token logic, which is the 
+link between the NEAR NEP-141 tokens and Aurora ERC-20 compatible tokens.


I have the next interpretation of this paragraph. Please check if this interpretation is correct.

We have the Aurora ERC-20 tokens as a concept on Aurora Blockchain. And this NEP-141 logic in Aurora Engine contract provides a way to interact with Aurora ERC-20 tokens from NEAR as with NEP-141 tokens. But anyway, from NEAR point of view, all the Aurora ERC-20 tokens are the same NEP-141 token with the address aurora.

The second interpretation. We have some NEP-141 tokens on Near and we have a correspondent official Aurora ERC-20 token for the origin NEP-141 token on Near. And this NEP-141 logic in aurora engine contract is somehow responsible for creating a link between these two tokens on Near and Aurora blockchains.

It will be nice to have a description of how it works or a link on the corresponding paper.

olga24912 · 2023-05-02T11:19:25Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+
+* improved security (due to a number of vulnerabilities found recently)
+
+* more flexible changes (this will not require updating the entire aurora contract)


Do I understand correctly that these changes improve security only by providing a way to update only the part with NEP-141?

I understand the paragraph in the next way. In practice, the NEP-141 logic is a difficult part that can contain different bugs and vulnerabilities. And for improving security, we want to have a way to fix bugs and update this part as fast as possible. Updating the whole aurora engine is complicated and slow and because of this, we want to separate this part to the external contract.

Is it important for me to understand the vulnerabilities? Or the only thing that I need to know is the existence of them?

olga24912 · 2023-05-02T11:29:44Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+third-party contracts through cross-contract calls.
+
+Putting the logic of NEP-141 into a separate contract solves important 
+tasks, the cornerstone of which is the interaction between contracts:


Don't get the part the cornerstone of which is the interaction between contracts. Do we mean that this separation of NEP-141 improves somehow the interaction between some contracts? In that case, I don't get what kind of contracts. Or do we mean that it is possible to separate the NEP-141 logic because we can interact with another contracts? Or is the main point of this phrase that cross-contract interaction is something difficult and we should pay extra attention to these changes?

olga24912 · 2023-05-02T11:38:01Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+
+It is **worth noting** that the new contract with the NEP-141 logic is 
+available for interaction outside of the Aurora contract. This means 
+that user or third party contract can access it from direct calls to new contract. 


So, do we want users to communicate with aurora-eth-connector directly? I don't get whether this ability is a good or bad thing.

olga24912 · 2023-05-02T12:05:46Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+
+Any reading or writing of data and use of functions that relate to 
+NEP-141 within the entire Aurora Contract must be rewritten to 
+cross-contract calls to the new contract. And in the future removed.


What is the list of the functions which should be rewritten?

olga24912 · 2023-05-02T12:07:59Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+
+* that all NEP-141 data has been transferred
+
+* the inability to write and read NEP-14 data simultaneously with the


Suggested change

* the inability to write and read NEP-14 data simultaneously with the

* the inability to write and read NEP-141 data simultaneously with the

olga24912 · 2023-05-02T12:13:42Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+This splitting of logic implies that the end user retains the Interface 
+(API) when interacting with the Aurora contract. The end user 
+will not notice any changes and is not required to make any edits or 
+changes to interact with the Aurora contract.


Not exactly. The user can notice that now the operation will finish in another block. And as was already mentioned in this AIP, now the failed transactions can return the success status. Some third-party contracts can rely on that.

olga24912 · 2023-05-02T12:19:17Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+One of the important aspects of security is that the new contract will 
+not be available for interaction to other contracts or users, except 
+for the Aurora contract. From the point of view of the external 
+interface of interaction with the Aurora contract, nothing will change.


Do we want users to communicate with a new contract or not? It was mentioned a few times that NEP-141 logic will be completely removed from aurora-engine contract in the future and completely switch to the new contract.

The Contract interface is public, as mentioned there are a few special methods only for Aurora Engine.
So any user can freely use the new contract.

olga24912 · 2023-05-02T12:25:49Z

AIPs/aip-split_nep141_logic_outside-of_engine.md

+cross-contract calls, as well as using the `near-sdk` implementation. 
+Previously, the `no-std` implementation was used, taking into account 
+the reduced gas cost. However, these costs are expected to be 
+negligible compared to the benefits.


Do you have any calculations of the gas cost before and after?

The gas costs were checked via Engine tests, and it's minor gas cost changes. Unfortunately, it's hard to calculate the concrete gas cost, because it's very dependent on Engine releases.

mrLSD added 3 commits July 20, 2022 18:51

Initial notes

d315010

Abstract section

433a352

Added: Motivation & Backwards Compatibility

9b245f1

mrLSD added the documentation Improvements or additions to documentation label Jul 28, 2022

mrLSD self-assigned this Jul 28, 2022

mrLSD changed the base branch from master to cross_contract_calls July 28, 2022 11:32

mrLSD added 2 commits July 28, 2022 18:10

Rational

d7a8072

Security Considerations

e93e07e

birchmd requested changes Aug 2, 2022

View reviewed changes

Small fixes. First points pf Specification

602dca4

birchmd reviewed Aug 2, 2022

View reviewed changes

AIPs/aip-split_nep141_logic_outside-of_engine Outdated Show resolved Hide resolved

mrLSD and others added 2 commits August 2, 2022 20:27

Update AIPs/aip-split_nep141_logic_outside-of_engine

e0777b9

Grammar changes Co-authored-by: Michael Birch <birchmd8@gmail.com>

Extend specification

961eab3

joshuajbouw suggested changes Aug 3, 2022

View reviewed changes

mrLSD added 4 commits August 4, 2022 16:42

Phrases reformulation

933780e

Data migration

c14498d

Specification: small changes with formulations

bf12a15

Rename AIP file

d15d167

mrLSD mentioned this pull request Sep 21, 2022

Split NEP-141 logic aurora-is-near/aurora-engine#607

Open

Extended Migration algorithm

34ac130

mrLSD marked this pull request as ready for review November 21, 2022 23:07

mrLSD requested review from a team and artob and removed request for a team November 21, 2022 23:07

mrLSD added 2 commits April 24, 2023 19:35

Changes for pending period

9059da8

Extgend functions set for aurora-eth-connector contract

078fc96

olga24912 reviewed May 2, 2023

View reviewed changes

mrLSD mentioned this pull request Aug 22, 2023

Fee integration Near-One/aurora-eth-connector#62

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split NEP-141 logic outside of Engine #5

Split NEP-141 logic outside of Engine #5

mrLSD commented Jul 28, 2022 •

edited

Loading

birchmd Aug 2, 2022

mrLSD Aug 4, 2022

birchmd Aug 2, 2022

birchmd Aug 2, 2022

mrLSD Aug 2, 2022

birchmd Aug 2, 2022

mrLSD Aug 4, 2022

birchmd Aug 2, 2022

joshuajbouw Aug 3, 2022

joshuajbouw left a comment

joshuajbouw Aug 3, 2022

olga24912 May 2, 2023

olga24912 May 2, 2023

olga24912 May 2, 2023

olga24912 May 2, 2023

olga24912 May 2, 2023

olga24912 May 2, 2023

olga24912 May 2, 2023

olga24912 May 2, 2023

olga24912 May 2, 2023

mrLSD May 2, 2023

olga24912 May 2, 2023

mrLSD May 2, 2023


		* improved security (due to a number of vulnerabilities found recently)

		* more flexible changes (this will not require updating the entire aurora contract)


		* that all NEP-141 data has been transferred

		* the inability to write and read NEP-14 data simultaneously with the

Split NEP-141 logic outside of Engine #5

Are you sure you want to change the base?

Split NEP-141 logic outside of Engine #5

Conversation

mrLSD commented Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuajbouw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrLSD commented Jul 28, 2022 •

edited

Loading