feat: add std.PrevRealm

[albttx]

Description

Currently the p/demo/grc/grc20 package and r/demo/foo20 contract use std.GetOrigCaller for checking the owner, this could lead to potential security issues (eg: phishing).

func (t *userToken) Transfer(to std.Address, amount uint64) error {
	owner := std.GetOrigCaller() // vulnerable
	return t.admin.transfer(owner, to, amount)
}

flowchart LR
   A[user1.gno] --> B("realm1.gno (caller is user1.gno)")
   B --> |"call unsafe package"| C[p/malicious]
   C --> |"call foo20.Transfer and steal tokens"| D("foo20.gno")

Loading

By using std.GetCaller, we fix this situation.

func (t *userToken) Transfer(to std.Address, amount uint64) error {
	owner := std.GetCaller()
	return t.admin.transfer(owner, to, amount)
}

NOTE: The changes in the grc20 packages and foo20 will be executed on another PR to keep this one small

How it's works

Add std.GetCaller which return the latest caller.

This should be mostly used on grc20 package rather than std.GetOrigCaller

How it's works

GetCaller already return the Caller, When the caller is a Realm, return the realm, if not, return the calling user.

Examples

User calm realm, that call realms

flowchart LR
    A[user1.gno] --> B("realm1.gno (caller is user1.gno)")
    B --> C("r/realm2 (caller is realm1.gno)")
    C --> D("r/realm3 (caller is realm2.gno)")

Loading

User call Realm that call only packages

flowchart LR
    A[user1.gno] --> B(realm.gno)
    B --> C("p/foo (caller is user.gno)")
    C --> D("p/bar (caller is user.gno)")

Loading

User call Realm that call packages and realm

Noticed that on r/bar the caller is realm.gno

flowchart LR
    A[user1.gno] --> B(realm.gno)
    B --> C("p/foo (caller is user.gno)")
    C --> D("r/bar (caller is realm.gno)")

Loading

How has this been tested?

see tests...

go test -v ./gnovm/tests/ -run "/zrealm_crossrealm11.gno"
go test -v ./gnovm/tests/ -run "/zrealm_crossrealm_exploit1_stdlibs"

[grepsuzette]

I like this.
Maybe it should be called GetLastCaller().

[tbruyelle]

I like this. Maybe it should be called GetLastCaller().

That makes sense since we have also GetCallerAt(position).

But on the other hand, I'm wondering if having all these methods to access to the callers stack is really useful. Why a contract would need to access the nth caller ? Sounds like an open-bar for security breaches no ?

[r3v4s]

We do definitely need this kind of stuff.

Could add TestSetCaller?? (just like TestSetOrigCaller)
It might be useful when testing.

https://github.com/gnolang/gno/blob/master/tests/imports.go#L537

[albttx]

Maybe it should be called GetLastCaller().

I get your point, but it's semantically wrong i believe.

user.gno -> daoX -> foo20

• user.gno is the GetOrigCaller.
• for daoX, user.gno is the Caller
• for foo20, daoX is the Caller, the "LastCaller" would be user.gno logically, (user.gno is still the GetOrigCaller of course)

It would made sense if the function was GetLast{Realm,Package}

@grepsuzette wdyt ?

[albttx]

Could add TestSetCaller?? (just like TestSetOrigCaller)

This is more complicate than it's look, TestSetOrigCaller just force the (*Machine).Context.OrigCaller,

GetCaller() is based on frame[i-1].LastPackage.GetPkgAddr(), which use DerivePackageAddr() on the pkgName, so if i force and edit of the frame package name, i might have side effets!

Maybe we should consider adding the Caller directly in the Context ? But the context is attached to the Machine, which is kind of annoying...
Or we can add it into the Frame?

[r3v4s]

Could add TestSetCaller?? (just like TestSetOrigCaller)

This is more complicate than it's look, TestSetOrigCaller just force the (*Machine).Context.OrigCaller,

GetCaller() is based on frame[i-1].LastPackage.GetPkgAddr(), which use DerivePackageAddr() on the pkgName, so if i force and edit of the frame package name, i might have side effets!

Maybe we should consider adding the Caller directly in the Context ? But the context is attached to the Machine, which is kind of annoying... Or we can add it into the Frame?

Hmmm.. I think we better leave gno context structure as it as for now, and find ways to spoof or mock the frame with least side effetcs

As you said touching context will be super annoying and likely to have more side effects

[piux2]

I agree; changing context dynamically in the call path makes the contracts hard to audit and opens doors for security issues.

In typical use cases, OrigCaller refers to the user account that signs the original message, not a contract on the call path. It equals EOA in Solidity.

In the file test, OrigCaller is the main package. It simplifies the testing process since we don't need to sign the message to initiate a call to contract from EOA. TestSetOrigCaller is only used to replace the main package for the admin user for testing purposes.

[moul]

Please share your thoughts on PR #683.

[tbruyelle]

Question: What do we want as ouput of std.GetCaller when it's executed by a package?

That's an interesting question, can a package be considered as a caller or not ?

I had the similar question for #495 for std.AssertOriginCall function. Basically, should std.AssertOriginCall panic if called inside a package called by a realm ? If yes, that means we consider the package as a caller, and as it's not the origin caller, the function panics.

[grepsuzette]

Perhaps the name is not very important for now.

@albttx I will try to rephrase your use case.

There is a user Bob, Bob represents a DAO on GNO, this DAO exists through a realm. Last week the DAO decided to emit GRC20 tokens and to airdrop them to 100 random users of r/boards.

• You're saying that in the code implementing this last sentence, the call std.GetOrigCaller() should play no part and should not be called. Correct?
• But that std.GetOrigCaller() is of course useful in general. Correct?

If that's correct, I'm wondering if we can think of scenarios where std.GetCallerAt() would be useful.
I'm of course trying to make an opinion for #683.

[albttx]

Hey @grepsuzette, no, here is the current situation

See here the grc20 code:

• https://github.com/gnolang/gno/blob/master/examples/gno.land/p/demo/grc/grc20/user_token.gno#L30

• https://github.com/gnolang/gno/blob/master/examples/gno.land/r/demo/foo20/foo20.gno#L51

The user_token check the GetOrigCaller

• a user.gno, the only member of the DAO (to make it simple)
• gno.land/r/the-corp/dao a realm for a DAO, (wallet the-corp-dao.gno)
• gno.land/r/the-corp/foo20 a $FOO GRC20 realm

Some infos:

• The DAO have a treasury of 1_000_000 $FOO
• user.gno have 0 $FOO

So now let's say, user.gno want to spend 200_000$FOO to grepsuzette.gno via the DAO

So, the tx will look like:

user.gno -> gno.land/r/the-corp/dao -> gno.land/r/the-corp/foo20

1. So user.gno call the DAO realm, (GetOrigCaller == user.gno)
2. DAO call FOO.Transfer() GetOrigCaller == user.gno, so we'll fail on insufisent fund, because the caller is user.gno where it shoud be the-corp-dao.gno

This PR adding std.GetCaller fix this issue.

[moul]

Blocking until we choose the best direction for this in #683.

[anarcher]

Personally, I like the name std.CallerAddress() (in case we need std.Caller() -> Caller (sturct or interface) later... :-))

[moul]

std.GetCaller().Addr()?

[albttx]

linked to #634

[jaekwon]

On another note, I suggest we revisit the API design and propose an alternative: std.RealmAt(X) as suggested by Jae. Here, std.RealmAt(0) would return r/boards, and std.RealmAt(1) would return user1.gno.

but what's the reason for needing that? first I would get a good use-case that isn't addressed by existing functions, and make sure this use-case is definitely something we want to support, rather than an anti-pattern.

[jaekwon]

reading it once more now.

[jaekwon]

i don't understand why it needs to be empty. what happens without this line?
Maybe it's solved by the if-else chain i suggested above. it seemed like it was unnecessarily setting lastPkgPath before.

[albttx]

std.Realm return a Realm{}

type Realm struct {
	Addr    crypto.Bech32Address
	PkgPath string
}

in the case we return a user, we want PkgPath to be empty.

Without this line, we could return a Realm containing a user address and gno.land/r/demo/xxx which is the "Current realm".

This condition won't be present in the next function std.Realm

[moul]

For accounts, it's a good idea to have an empty variable. However, if default realm implementation is desired later, it could change to gno.land/r/moul. What is the current value before erasing it?

I like using if PkgPath != "" for distinction, similar to if err != nil. We can update the method or choose a new variable when accounts have a path.

Waiting for your response, but currently inclined to maintain the current behavior.

[albttx]

Hi @moul I'm not sure i understand your question, but here is a better explanation

user1.gno -> gno.land/r/moul/sayhi

The Frames will be

• gno.land/r/moul/sayhi
• user1.gno

so when we will finish the iteration loop, lastCaller will be user1.gno, and pkgPath will be gno.land/r/moul/sayhi, ( frame[i].LastPackage.PkgPath)

So we use if ctx.OrigCaller == lastCaller to empty it

Is it more clear ?

[moul]

I was asking what was the raw value before erasing it. Checking if it makes sense to be kept somehow.

[jaekwon]

Really close to final; just have the comments written so far.
If nobody objects to new changes, feel free to merge if all comments are addressed.

[r3v4s]

Just wanna check

PrevRealm is like eth's msg.sender
GetOrigCaller is like eths' tx.origin

Did I understood correctly?

[r3v4s]

It works perfectly fine for actual gno running with gnoland, and interacting with gnokey

However, because gno test uses its own context, PrevRealm() doesn't work on test case.

gno test -root-dir ~/gno -verbose=true ./
Test OrigCaller: g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm

=== RUN   TestFunc
outer_std.GetOrigCaller(): g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm
outer_std.PrevRealm(): struct{("g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm" std.Address),("" string)}
called OuterToInner()

calling inner.Inner() from outer
inner_std.GetOrigCaller(): g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm
inner_std.PrevRealm(): struct{("g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm" std.Address),("" string)}
called Inner()

FINISH OuterToInner()
--- PASS: TestFunc (0.00s)
ok      ./. 	0.49s

So I'v made little helper #891