gnolang / gno

Gno: An interpreted, stack-based Go virtual machine to build succinct and composable apps + Gno.land: a blockchain for timeless code and fair open-source.
https://gno.land/
Other
878 stars 361 forks source link

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

Open leohhhn opened 8 months ago

leohhhn commented 8 months ago

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.

moul commented 8 months ago

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 commented 7 months ago

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 commented 7 months ago

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 :wink:, 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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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

thehowl commented 7 months ago

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