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

Replacing GetOrigCaller() with PrevRealm.Addr() #1577

Closed
leohhhn opened this issue Jan 23, 2024 · 9 comments
Closed

Replacing GetOrigCaller() with PrevRealm.Addr() #1577

leohhhn opened this issue Jan 23, 2024 · 9 comments

Comments

@leohhhn
Copy link
Contributor

leohhhn commented Jan 23, 2024

Description

GetOrigCaller, in pair with AssertOriginCall & IsOriginCall, should be replaced with PrevRealm.Addr() to accommodate calls that are not made only by EOAs. This is because limiting calls only to EOAs blocks a lot of useful functionality, including account abstraction, smart contract wallets, situations where you would want a realm/package to call another realm/package, etc.

As is the case with Ethereum's msg.sender & tx.origin, where tx.origin was kept as a legacy functionality, there is not much use for the GetOrigCaller function - we should replace its usage with PrevRealm.Addr().

I propose for the beginning that we replace all calls to GetOrigCaller with PrevRealm.Addr(), where the intended functionality is to get the previous caller of the function. Also, I propose removing any AssertOriginCall checks in the examples/ folder where it makes sense.

@leohhhn leohhhn changed the title Replacing IsOriginCall with PrevRealm.Addr() Replacing GetOrigCaller with PrevRealm.Addr() Jan 23, 2024
@leohhhn leohhhn changed the title Replacing GetOrigCaller with PrevRealm.Addr() Replacing GetOrigCaller() with PrevRealm.Addr() Jan 23, 2024
@moul
Copy link
Member

moul commented Jan 23, 2024

Before making changes everywhere, I would like to request people to provide examples of proper usage of GetOrigCaller. This way, we can ensure that we only modify what is necessary, rather than altering everything.

@piux2: I believe you mentioned examples during a previous call.

@piux2
Copy link
Contributor

piux2 commented Jan 31, 2024

AssertOriginCall is used to ensure there is no middleman. GetOriginCaller validates who signed the transaction, while PrevRealm is used for whitelisting contracts.

These functions can be used separately or combined to achieve varying levels of security.

For example, consider contracts that allow users to claim NFTs and AirDrops from a pre-screened wallet list. In this scenario, GetOriginCaller is necessary to validate the request.

However, the claim request can be sent either directly from the user to the Owner's contract or through a third-party entity, such as a token distributor or an NFT auction house.

The Contract Owner has further two options:

To prevent middleman involvement, use GetOriginCaller + AssertOriginCall.
To allow pre-selected or registered third parties, use GetOriginCaller + PrevRealm.

@jefft0
Copy link
Contributor

jefft0 commented Jan 31, 2024

I and many developers have not been neck deep for years in obscure smart contract security concerns. The selling point of Gno.land is that it doesn't have a steep learning curve. IMO 😉, the "safest" of these security behaviors should be enabled by default, without the realm developer needing to figure out which of these weird functions they have to call at the top of every public realm function. That's the job of the Gno.land infrastructure. It should be built-in, with a way for advanced developers to override if they have studied the security details and want to change it.

@piux2
Copy link
Contributor

piux2 commented Jan 31, 2024

@jefft0
Thanks you for the feedback. Good suggestion!
What do you think about creating standard rules and policies to abstract away these low-level checks?

@jefft0
Copy link
Contributor

jefft0 commented Feb 1, 2024

Here's the idea I had in mind: The default is the check you mentioned to "prevent middleman involvement". The VM enforces this without the realm function needing to call a function like AssertOriginCall. However, the first call of the realm function can be an override to define a "white list" or whatever. When the package is added to the realm, each public function is scanned for these enforcement calls. If present then these override the default behavior.

@tbruyelle
Copy link
Contributor

Additional note: the removal of GetOrigCaller implies also the removal of TestSetOrigCaller.

@thehowl
Copy link
Member

thehowl commented Feb 19, 2024

The default is the check you mentioned to "prevent middleman involvement". The VM enforces this without the realm function needing to call a function like AssertOriginCall.

There's another need we're balancing here: the desire to build a platform which is very powerful in terms of "composability".

Aside from the technical complications of having something like "AssertOriginCall" by default and disabling it ad-hoc, it just means users or other contracts cannot "script around" a method. AssertOriginCall is like placing your data directly in the HTML of a page (if someone wants it, they have to scrape it => in Gno, they have to call it with MsgCall in a tx), the opposite is having the data in a public API (=> you can just call a realm's function from another realm or MsgRun).

@moul
Copy link
Member

moul commented Oct 15, 2024

Can we centralize all the std refactor issues into a single one to provide a global solution?

Maybe #1475?

@thehowl
Copy link
Member

thehowl commented Oct 31, 2024

centralizing on #1475

@thehowl thehowl closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

8 participants