Token Allowance Interface Proposal

[blissful lake]

Current token allowance interface includes two functions:

fn increase_allowance(env: Env, from: Address, spender: Address, amount: i128);
fn decrease_allowance(env: Env, from: Address, spender: Address, amount: i128);

I suggest we:

1. Unify these under one function to closer resemble the ERC20 approve() function
2. Add an expiration ledger for the allowance both as a safety measure for the user and also to allow the allowance to be stored as a temporary entry

fn approve(env: Env, from: Address, spender: Address, amount: i128, expiration_ledger: u64);

[lone fog]

a single function is dangerous as it has very unclear semantics

(and IIRC there is a history of bugs due to that)

expiration ledger suggestion is interesting because the last time we've discussed this it seemed like the main use case for allowance in general is stuff like subscriptions, where unlimited allowance is needed

temporary allowance seems to me like an anti-pattern given existence of auth next (why create a short-time allowance when you can do a direct transfer?)

[steep sequoia]

1. If you merged those two to match ERC20, it would be called approve, not allowance.
2. They were split into two to avoid a dangerous scenario with reducing allowances - https://blog.smartdec.net/erc20-approve-issue-in-simple-words-a41aaf47bca6

[blissful lake]

(why create a short-time allowance when you can do a direct transfer?)
does temporary have to be "short"? even if I use it for subscription like use cases I can set it to a year

[lone fog]

hmm, I guess it's ok if you write the machinery that bumps allowances for your customers

actually, the issue is that bump would need to be coupled with allowance duration

[brazen trail]

Expiring evicted allowances are sort of a surprising UX. Network looks like allowance is gone. THen it comes back. Obviously this is a problem with all entries that get evicted, but allowances are sort of annoying and footgunny specifically.

You might allow someone, later see no allowance, then increase allowance again, and 💥 without realizing you've allowed them to take too much.

So in that way temporary allowances are safer.

[lone fog]

FWIW with mergeable allowances would probably be 'take latest' (i.e. ignore restored entry)

[brazen trail]

Maybe, but then at that point, isn't temporary the same thing?

[lone fog]

so what should be the behavior if the allowance time bound exceeds max temp entry lifetime? just set it to the max and hope that the party that uses allowance will bump it?

[lone fog]

not necessarily; merge behavior kicks in only when the entry has been recreated on-chain. the main purpose would be to just bring it back without bothering the approver

[brazen trail]

But now the story is more confusing.

Sometimes allowances disappear, other times they don't.

I allowed you an extra $5, and all the previous allow I gave you is gone.

Another time it isn't gone, because I didn't allow after eviction.

Confusing UX with money drains confidence.

[blissful lake]

^ this is part of why I suggested moving back to a single value approve() function. The delta functions have weird semantics when allowances disappear

(although @steep sequoia's point about safety should be taken into consideration)

[lone fog]

so what about the exceeding the max possible expiration time? isn't that confusing as well?

[brazen trail]

Setting to max in that case sounds fine.

[lone fog]

also, with expiration ledger I agree that we no longer can have increase/decrease functions. I think the safest option would be to create a unique id for allowance and pass it to transfer from.

[blissful lake]

@lone fog I think the token contract should enforce that the allowance expiration ledger does not exceed the max possible expiration time

[lone fog]

but then the max possible allowance time is the max temp entry life time, i.e. 1 year (or maybe even less)

i.e. rent bumps will be pointless

[brazen trail]

1 year seems like a long time for an allowance?

[blissful lake]

@lone fog no one has to bump anything here. the user can just call approve() with a different expiration ledger

[brazen trail]

If that covers 95% of use cases, maybe sufficient?

[lone fog]

I mean, if the main case is subscriptions, then I'd say it doesn't cover it that well. but honestly I'm not really sold on this use case either, so probably whatever, I'm ok with temp only allowances

[brazen trail]

What do we know will be the maximum temp entry life time at the moment? Is 1 year above a good estimate, or random guess?

[lone fog]

I believe 1 year has been planned before, though maybe we need to be more conservative, given that we have to provide expiration ledger guarantees and we don't want to discount early entries that much

[blissful lake]

I'm ok with having allowances that are limited to up to 1year

[brazen trail]

@near sphinx I think I heard you say 1 year for eviction epochs, which lines up? I might have misunderstood though.

[near sphinx]

I don't know if max temp duration and eviction granularity is the same

I can try to reason it out from first principles but I don't quite follow why it matters

here are values in Garand's recent PR:

    // 1 year in ledgers
    static constexpr uint32_t MAXIMUM_ENTRY_LIFETIME = 6'312'000;

    // Live until level 6
    static constexpr uint32_t MINIMUM_RESTORABLE_ENTRY_LIFETIME = 4096;
    static constexpr uint32_t MINIMUM_TEMP_ENTRY_LIFETIME = 16;

i.e. there doesn't seem to be a max temp entry lifetime, just a minimum, and a max entry lifetime in general (which is 1yr)

[green sequoia]

so is this to refactor into the SAC?

[blissful lake]

@green sequoia this is about the token interface which also affects the sac

[green sequoia]

yeah okay

for a moment i thought it was gonna be a seperate interfacce just for allowance

and got confused

[gaunt osprey]

How about we track allowances along with cumulative spent amount? Maybe a tuple struct struct Allowance(i128, i128); or explicitly defining the fields like below? Will that help with that attack scenario?
`// Track allowance and amount spent together.
struct Allowance {
allowance_amount: i128,
spent_amount: i128
}

pub fn write_allowance(
e: &Host,
from: Address,
spender: Address,
new_allowance: Allowance,
)`.

fn spend_allowance will keep updating the cumulative total spent_amount and we won't allow if it goes beyond allowance_amount.

[lone fog]

this doesn't work well with temp allowances

[brazen trail]

As far as I can tell from the above conversation, there's no blocking reasons for simplifying the interface to a single approve function, and the existing two function design is flawed for Soroban in ways it isn't for Ethereum due to the footgun created by expiring entries.

If we change to one function, we could have a best practice of telling people to reduce their allowance to zero before changing it, or optionally require the current allowance amount when changing the allowance amount. Both these ideas are in that article linked above too.

As @blissful lake suggested simply:

fn approve(env: Env, from: Address, spender: Address, amount: i128, expiration_ledger: u64);

Or the safer variation outlined in the article:

fn approve(env: Env, from: Address, spender: Address, currentAmount: Option<i128>, newAmount: i128, expiration_ledger: u64);

[zealous dagger]

@brazen trail could be possible to make the allowance function to just overwrite the old value set? because if the good practice is to move the value to zero before updating it the result will be the same, and if they are required to get the current value before updating it then they could just calculate the total if they just wanted to increase the current allowance

[brazen trail]

Setting the allowance without first setting it to zero, or setting it based on the current allowed amount, can be exploited. The link here has an example: #1114336575438999583 message. The good practice of setting to zero, or checking the current value in the function, address it.

[lone fog]

I don't think setting allowance to 0 achieves anything - it still can be frontrun in the same way

the old value kind of helps, but the correct flow seems very complicated (like if my tx that tries to decrease allowance fails, then what?)

[brazen trail]

See the article above that @steep sequoia shared. The relevant snippet for how 0 achieves safety is:

Since Bob is able to transfer 1000 + 300 tokens when allowance is being changed from 1000 to 300 tokens, then changing the allowance from 1000 to 0 tokens means that Bob will be able to transfer at most 1000 tokens. After the first call of the approve function, Alice should check whether her balance has decreased. If everything is ok, she can call the approve function again to change the allowance from 0 to 300.

[lone fog]

but what if it's not ok?

the same goes for the amount check

the 0 flow is basically the same, but with even more complexity as some off-chain balance check has to be performed...

[brazen trail]

If the spender spends an amount of the 1000 before the 0 applies, then they spent an amount the user had allowed them to 🤷🏻‍♂️. No different to if the user had submitted decrease allowance and someone had spent the amount before the decrease went through.

[lone fog]

right... but I guess in case of decreasing allowance it's 'fire and forget', while other scenarios are more involved. would you never allow anything to this spender anymore?

[brazen trail]

It's not really fire and forget with state expiry, because the wallet needs to check there isn't a restorable allowance. Inc/decr don't offer the same convenience in Soroban with state expiration.

Or at least an edge case exists that is easy to miss and then get exploited.

The safe approval method which takes the current value seems pretty effective?

[lone fog]

sure, what I mean is that it's fire and forget in the world without expiration.

[zealous dagger]

mmm I mean I find the example in the article as a case where the spender actually spent what the sender is actually approving to, but I guess that if we want to prevent that case then providing the current value is the only way the way to go so in that example instead of the spender using 1300 it only uses 1000

[lone fog]

with expiration it (=increase/decrease interface) doesn't work well. I would probably prefer an option where every allowance has an id and then the only thing you could do with allowance is to revoke it (i.e. if you want to modify it, you create a new allowance, which signifies the fact that you need to get rid of the old one)

the allowance with current amount would probably work as well, but it seems to provide more ways to mess up (e.g. by ignoring the old value unconditionally)

[brazen trail]

I like the idea of using an ID in principle but I think it would be difficult to integrate across contracts. The current value in the approve function approach complicates only a single function, where-as an ID complicates every interaction relating to the allowance. Any call tree that has an allowance consumption deep within it would require passing the ID all the way down. Even the current value in the approve might be too not-integratable. The 0 option while it feels messy, I think keeps contracts easier to integrate.

[lone fog]

I'm not sure if making using allowance 'harder' is necessarily a bad thing. an explicit id would obviate the need to create an allowance to use the contract - currently that can only be implied from some external documentation

[brazen trail]

That's a good point.

In this idea are allowances single use? THey should still be multi-use if not consumed completely yes?

[lone fog]

yeah, you still can consume it in an arbitrary number of batches.

[blissful lake]

I think that ids create some additional complexities. The allowed entities will need to do book keeping that they don't right now. And users can't get a simple straightforward answer to the question of "what's my pending USDC balance to this protocol" - there needs to be a indexing service that aggregates allowances in order to answer that question

[lone fog]

well, there is no infrastructure for this in general, so something has to be done anyway. again, I can't view allowances as a core feature and I don't think it should be used arbitrarily everywhere

and it's also even harder to imagine the case where you'd have multiple allowances given to the same contract (and in that case ids arguably make things more clear)

[brazen trail]

It's also novel, in the sense no one else is doing it? So there's unknown risks.

Would APIs need to pass around potentially multiple allowance IDs? Could get clunky.

@lone fog I don't think an ID prevents the exploit case though in a way that reduces the number calls or interactions. For example, if I, Account A, wish to grant spender S 1000, I first have allowance ID S1 with 1000. Then, if I wish to reduce spender S to 300, I need to revoke S1 first, wait and confirm the revoke succeeded and that no funds were taken from it, then only after doing that I can create allowance S2 with value 300. This is the exact same series of steps as setting the allowance to 0 first.

[lone fog]

yeah, sure, it's going to be awkward in any case. I guess the main benefit would be that it enforces it in a way - it's pretty obvious that if you create an allowance for 1000 and an allowance for 300 your total allowance is 1300. it's not obvious if you set it to 1000 and then to 300. I think this encourages better contract design - like if you need to pass around multiple allowance ids then chances are you're doing something wrong and should probably not be using allowance in the first place

[hidden cargo]

leigh asks: should we move ahead with approve and recommend setting to zero before changing the value?

[steep sequoia]

Some questions (cc @lone fog )-

1. Should the expiration ledger be an absolute ledger number, or the number of ledgers to expire from the current ledger. I think it should be the latter.
2. Should the expiration ledger for the allowance be exposed to users of the contract? I think no, but this could be confusing. Querying an expired allowance will just return 0, so maybe that's sufficient?

[lone fog]

relative ledger seems footgun-y as the signature will be tied to the execution time (and not the signature time). i.e. if I sign allowance for 10 minutes now it still might be used tomorrow. this is not an issue for soroban auth (as signatures expire anyway), but might be an issue if invoker auth is used

maybe it's not a big deal though... not sure really

as for the users, I'd say no - they should just be using transfer/burn_from anyways

i.e. I think regular accessor is good enough