RFC Proposal for a new stack/frames management

[moul]

Context

The stack/frames is an integral part of the virtual machine (VM) and the language, and we rely heavily on it. Additionally, stack/frames provides a context for smart-contract developers, enabling them to access useful information such as the original caller or to determine if a contract is being called through another one.

The current implementation of the helpers GetOrigCaller and GetCallerAt (found at https://github.com/gnolang/gno/blob/41af8f6daa54e949c77e04b375b7f3dd6ae0df71/stdlibs/stdlibs.go) is limited for advanced rules. It also relies on fixed positions in the stack, and can lead to inconsistencies when the runtime is configured differently, such as when unit testing with _test.gno or _filetest.gno.

Continuous efforts are being made to enhance various components and expand functionalities, as evidenced by issues #335, #393, #402, #644, #667, #495.

Proposal

I propose a comprehensive solution that could address all the topics discussed, or serve as a foundation to build upon.

The stack entries in this context are immutable, but custom rules can be added to the stack. This is similar to how UNIX processes work with their Process ID (PID), forks, and UID management. Additionally, LXC provides an extra virtual layer for isolation and interoperability.

To enhance #644, consider renaming ExecAsPkg to SetUID or enabling the called contract to recognize the calling contract as its own caller by adding a new stack entry for the passed closure.

To make the necessary changes, replace std.GetOrigCaller and std.GetCallerAt with std.Callers() std.Frames. This function returns a slice of callers, each represented by a simple struct that contains pid, uid, and StackType. The pid-like entries in the slice function like a process ID, while the uid changes only with helpers like SetUID. The StackType field facilitates the identification of transactions, contracts, VM events, and custom context entries.

Here is an example that demonstrates the use of std.Callers() std.Stack and its resulting structure:

{
   0: ["origCallerAddr", "origCallerAddr", "t_transaction"],
   1: ["fooPkgAddr", "origCallerAddr", "t_callcontract"],
   2: ["barEvent", "origCallerAddr", "t_bar"],
   3: ["bazPkgAddr", "origCallerAddr", "t_call_imported"],
   4: ["fooPkgAddr", "fooPkgAddr", "t_setuid"], 
   5: ["bazPkgAddr", "fooPkgAddr", "t_call_imported"],
}

To include a helper (gno.land/p/demo/rules) package for Gno, with clearly named helpers like rules.GetOrigCaller(stack), rules.IsDirectCall(stack), rules.IsIndirectCall(stack), rules.IsCalledThroughPkg(stack, "gno.land/r/foobar"). Additionally, there is an associated issue that can be found at #301.

WDYT?

[albttx]

After a lot of works on all ExecAsPkg, std.GetCaller here is my thought on this situation

What's the problem ?

The package gno.land/p/demo/grc/grc20 is currently using std.GetOrigCaller which return the address of the person that execute the tx.

Which could lead to a lot of security issue (phishing for example), see this article for example

So how do we manage the treasury of a realm ?

Solutions

❌ The first idea was std.ExecAsPkg (#644), which i don't believe it's a good solution anymore

I believe that by default that the caller should be the realm if the realm is a caller.

✅ IMHO #667 (std.GetCaller) the best solution.

there is an ongoing question about this feature, i'd love your inputs

Some proposals

By working on std.GetCaller, we discovered that the Context is attached to the *Machine and not to Frames, which is logic, but could be annoying.

• Add a FrameContext that contain the Caller, and could be useful for the future.
• Add std.GetCaller which return the caller feat: add std.PrevRealm #667
• Add std.GetCallers which return an array of all callers (not 100% sure about this)

Thoughts

• I don't this Realm writers should be aware of the Frame system.
• i don't like the gno.land/p/demo/rules idea, i believe it must be under std. and std.GetCaller and std.GetOrigCaller are enough for a realm writer.

[tbruyelle]

I share most of @albttx's opinion on this.

I don't think giving access to all the callers and frames to the smart contract developer is really needed. By restricting to the last caller via std.GetCaller(), we prevent the phishing attack mentioned above. But maybe it's too restricting, I lack xp in smart contract to be completely confident on this.

About std.ExecAsPkg, so if I understand correctly, by wrapping the call inside std.ExecAsPkg, you make the called contract believe he's under an origin call right ?
If this is correct, I think a better name should be std.ExecAsOriginCall(). I don't think SetUID is very meaningful for everyone (at least it's not for me ^^').

[moul]

Yes, I get your point. Imagine this scenario:

• gno.land/r/the-defi-company/foo20 is a grc20.
• gno.land/r/manfred/vault is my personal vault (could be a multisig too).
• gno.land/r/bob/routines contains helpers allowing to make complex flows in a single call instead of many transactions.

With SetUID, I can own foo20 tokens using my personal vault contract and my wallet, and use routines directly or through another SetUIDed contract.

By returning frames with both the caller and the current uid, we let to the contract the option to let the final user decide rules.GetUID() or make as you suggest with rules.GetCaller().

---

In my opinion, SetUID is a security issue that is more relevant to the developer rather than the end user.

It should be noted that the grc20 module is designed to be flexible and extended. Additionally, we have a banker module that provides increased security measures, especially when users need to specify and deduct tokens from their balance during a transaction. It's worth noting that the banker module is not exclusive to ugnot as any contract can mint new tokens (prefixed with contract) using it.

I propose implementing the SetUID feature and offering several helpers around the richer stack. One of which can retrieve the last caller as you suggested. Furthermore, we can explore options to increase auditability for advanced functions, such as highlighting the relevant code or using symbols to indicate the use of advanced features by the developer.

---

Edit: We have more flexibility thanks to a rich stack and something similar to SetUID. This allows the final grc20 developer to choose between OnlyDirectCall, AllowAnything or something intermediary by using the grc20.Approve(parentContract, limit) method with a helper in p/rules or p/grc20.

[albttx]

Ok, i see what's your point,

Let's me clarify, with moul.gno you want to be able to spent moul-vault.gno tokens, i see where you are going, but i don't believe a UID system is the right option.

With SetUID, I can own foo20 tokens using my personal vault contract and my wallet, and use routines directly or through another SetUIDed contract.

We don't need SetUID for that, this is what Allowance and Approve are for in ERC20 specs, and then use TransferFrom.

And you can easily add in your gno.land/r/the-defi-company/foo20 a whitelist of Callers

You could use something like that to exec as your vault

func Exec(fn func()) {
    if whitelistCallers.Contains(std.GetCaller) {
        fn()
    }
}

Also, in a phishing concern, a malicious realm could make advantage of a SetUID system and exploit that.

[moul]

Yes, I listed this example before your comment.

Clarification: Our discussion mainly focuses on returning the full call stack rather than just the last caller, while SetUID is a separate topic that can offer additional opportunities.

While both approaches provide the same level of security for the Allowance pattern, my approach offers an additional capability for developers to access the initial caller, whereas your approach restricts developers to using only the Allowance pattern.

Implementing the Allowance pattern everywhere, including non-GRC20 tokens, would result in a poor user experience, especially when dealing with multiple layers of imports. With numerous inter-contract interactions expected in Gnolang contracts, implementing the pattern repeatedly, including with proxy patterns, would be impractical and overly complex.

Blockchains are not made for DeFi, especially Gno. Let's ask the GRC20 developers to follow best security practices, while permitting other contracts like forums to use the origin caller.

Reminder: My proposal is to use std.Callers() std.Frames and gno.land/p/demo/rules for helpers, instead of std.Get*Caller.

---

SetUID is a separate topic that enables intermediary contract developers to affect the context and allows end-contract developers to decide whether to use that information or not. Let's defer the discussion on SetUID and concentrate on selecting a call stack retrieval strategy.

SetUID is still just an idea, not a feature. If implemented, it would warrant a more extensive frame history, given the potential similar needs with goroutines. Frame[-2] may not always be the wallet or a different contract.

---

Edit: Another proposal: Updating GetOrigCaller, GetCallerAt to return a Frame struct with additional fields instead of an std.Address. To return nil instead of panicking for GetCallerAt(len+1), so we can build the []Frame from userland. This would fulfill my expectations, be easier to understand, and result in fewer breaking changes while improving performance.

[piux2]

- Not sure Tx.Origin Fishing Attack applies to Gno

tx.Origin attack works because the Solidity contract's fallback function can be invoked without calling any method's name, like an anonymous function. From the end-user perspective, there is no difference between sending tokens to a person’s EOA or a phishing contract because they all send tokens to an address. Users thought they transferred tokens to a person, but in reality, they sent them to a phishing contract, which can use tx.Origin to construct a contract call in the fallback function to transfer funds to anyone and the message appears from Origin Caller to valuable contract.

Checking immediate msg.Sender in Solidity has their own trust and security trade-off.

In GNO, there is no contract fallback function (anonymous function) for end users to call upon. All function calls to the realm package require exported function name. Checking OrigCaller prevents malicious contracts in personating a request from end users.

[moul]

Although this particular attack may not apply, we should be prepared for other potential attacks that could arise.

I believe that we should not use this as a reason to remove an important feature for end developers. Even if we implement my suggestion with a richer stack and SetUID, we can still create an exceptionally secure flow for grc20.

I'll create a new issue to suggest security improvements.
Edit: -> #688

[anarcher]

IMHO, It seems that Signer in the Move language has a similar usage example about it. (I don't know Move) https://move-language.github.io/move/signer.html

A signer is somewhat similar to a Unix UID in that it represents a user authenticated by code outside of Move (e.g., by checking a cryptographic signature or password).

[ltzmaxwell]

Implementing the Allowance pattern everywhere, including non-GRC20 tokens, would result in a poor user experience, especially when dealing with multiple layers of imports. With numerous inter-contract interactions expected in Gnolang contracts, implementing the pattern repeatedly, including with proxy patterns, would be impractical and overly complex.

Blockchains are not made for DeFi, especially Gno. Let's ask the GRC20 developers to follow best security practices, while permitting other contracts like forums to use the origin caller.

This makes sense. For ERC-20, approve and allowance are mechanisms designed to solve specific problems. From a more general perspective, these mechanisms should ideally be supported by more fundamental and generic methods at the lower level.

[moul]

Feedback from the last private discussion:

1. It may be beneficial to return a structure with additional attributes instead of just an address. However, we need to define what those attributes should be. We can look at what Go, Unix, and other process management systems are doing. Additionally, Erlang has some great tools that we can consider.
2. The concept of uid could be interesting, but using the term pid is not ideal.
3. Instead of needing things like std.ExecAsPkg(callback) or std.Sudo(callback), we should add a way to include metadata in the context. We can do this with arbitrary tags like context.GetValue() and context.SetValue(). Then we can write helper libraries that provide official ways to communicate the calling context to the next contract. We can also create helper functions to search for specific tag names in any frame or to list tags for a particular frame.
4. In regards to helpers, address the issue of std.GetCallerAt panicking. Maintain the std package's minimalism and utilize a separate gno.land/p/rules package for more advanced filters. Only add new helpers to std when they provide a clear advantage and their definition is well-defined, such as issue feat: add std.PrevRealm #667.

[jaekwon]

I'm glad we're taking this slow.

Here's a related suggestion: https://github.com/gnolang/gno/pull/667/files#r1169454363 This is to implement Get[Realm]Caller().

There is a question of whether we want to return rich frame information. It's certainly useful for debugging panics in a program. But also, this could be implemented separately later with functions like "GetCallerFrames" or "GetCallerFrameAt".

So I suggest that we keep the functionality of GetOrigCaller and GetCallerAt() the same for now, and also to add GetRealmCaller(), where all *Caller[At] functions return an address for now, and that we spec out what a frame looks like in a separate PR and figure out how best to make it accessible to the programmer after.

[moul]

We discussed today about this topic (#667 (review)).

---

We should:

• Remove Get prefixes from std.Get helpers.
• Replace GetCaller* string, with Caller* Frame.

type Frame struct {
    PkgPath string
    Method string // maybe later
}

Consider introducing a new API for realm retrieval, excluding packages:

• std.GetRealmCallerAt(idx) to mimic std.GetCallerAt(idx), but omit p/ packages.
• i.e., std.GetRealmCallerAt(0) to return 'self' from a realm.