[spl-token] Add support for an owner of an account to disallow transerring tokens to the said account

[sushi-shi]

Rationale

Quite often different staking pools owning spl-token accounts have an internal logic dependent on how much tokens were transferred to those accounts. However, tokens can be transferred to those accounts without notifying said programs in any way, and if it isn't taken into consideration this might lead to inconsistencies inside said programs which easily can result in many kinds of vulnerabilities. To combat this, pools currently maintain their own internal balances, which are used alongside the spl-token account. This is unsatisfying, however, as it requires programmers to re-implement logic for spl-token (even if it is simple) over and over again.

Solution

As I currently see it, adding a new enum option (tentatively called Locked) with an instruction to Lock/Unlock accounts is backwards compatible (as size of enum will stay the same) and should be reasonably easy to implement.

And if an account is locked transfers to it are disabled. They can either be disabled completely and require to Unlock an account first or they can require two signatures, which is fine, as the second signature is going to be PDA in most of the use cases.

The second solution seems more ergonomic yet requires to change an existing interface a bit.

[askibin]

could you please give a concrete example when extra tokens break logic or lead to a vulnerability? there could be some state that depends on pool balance history (like average price or deposit amount), but it can still be updated next time a transaction is called.

[sushi-shi]

Imagine there were a pool in which you could stake your LP tokens to earn rewards in REW, and that for each second for each LP token you receive one REW. For it to be implemented we need to store a timestamp on when the pool was last accessed by a user. To provide rewards a naive solution would calculate a time difference between the current time and when the pool was last accessed by a user and multiply it by amount of LP tokens they have in their corresponding account (owned by a pool). And it would be wrong. As the user could've transferred tokens at the last second and our pool would think that this user had them for the whole time period and would reward them too much. And this is the simplest problem that might come up, there might be many more insidious and minute vulnerabilities that might crop up because of the expected and actual balance differences, especially if the logic in a pool becomes a bit less trivial. And by the end of the day, I don't really want to think too much whether it might result in similar problems or not (in the pools I implemented it could), I just want my wallets to contain what I expect them to have. No token less, no token more.

[jordaaash]

Sorry, accidentally closed 😅

As the user could've transferred tokens at the last second and our pool would think that this user had them for the whole time period and would reward them too much.

Contracts like SushiSwap's MasterChef generally solve this by assigning shares to the user based on the current state of the pool at the time of deposit. This prevents manipulating the rewards like you're describing. LP tokens themselves are a form of these shares. Can you say more about why a program would want to rely on the token balances instead of its own domain-specific business logic?

[askibin]

I see now where the problem comes from. If you transfer LP tokens to an individual custody per user when they stake their tokens and use that wallet balance to determine rewards, you will indeed have a problem.

The right way is to store a state per user and keep all LP tokens staked from all users in a single custody account. If someone deposits LP tokens to it, bypassing your program, they will lose them altogether and they won't affect rewards. Every time a user deposits/withdraw, you just update the state, i.e., assign rewards and update balance, and don't care about the amount in custody.

[sushi-shi]

@askibin

The right way is to store a state per user and keep all LP tokens staked from all users in a single custody account.

This introduces a single point of failure where if a bug were to be found, funds of all users could be jeopardized. While keeping an account per user would mitigate damage somewhat. Also "a state per user" most likely will closely resemble what spl-token does anyway. That's what I meant be re-implementing the logic again and again in OP post.

@jordansexton

Can you say more about why a program would want to rely on the token balances instead of its own domain-specific business logic?

Yes, of course! This domain specific logic will closely follow or will be highly dependent on what users staked and, hence, must have in their wallets. So why not use these wallets directly and maintain only a single counter, representing the total amount staked, using which shares could be calculated trivially by dividing?

As an example, Raydium here has to maintain balances for each user in a pool, doing most likely the same thing spl-token already does, so replacing it with a wallet per user seems like a more ergonomic and direct solution.

[askibin]

Maintaining a separate wallet per user doesn't release you from state storing requirements. You need to know when the last deposit was made, with how many tokens, and record accumulated rewards so far. Alternatively, you can record the only timestamp and payout past rewards every time the user deposits/withdraw from the stake. So, in both cases, you rely on some info stored per user, and in the first case, you have to store the balance as well. Not that I'm against the proposed feature though, it might still be cool to have.

[joncinque]

A couple of ideas:

1. What about freezing / thawing accounts? Your program can have logic for initializing / depositing into / withdrawing from special LP token accounts, which are frozen by the program. Whenever you issue new LP tokens, the program unfreezes, mints to, then refreezes the account. Only frozen accounts are accepted, and the freeze authority is a PDA, so you're sure that the program has full control over them. Instruction: https://github.com/solana-labs/solana-program-library/blob/cd8d79a2b4aa4f90c02514d762ab21023449b6cb/token/program/src/instruction.rs#L228
2. If there's any new logic needed, it can eventually be addressed as a new token program "extension", which is meant to cover any additional logic bits that can be enabled / disabled as needed. Here's my in-progress PR, still quite a bit to do for the framework, but it's getting there: https://github.com/solana-labs/solana-program-library/pull/2642

[joncinque]

This is covered by the confidential transfer extension in token-2022 with #3790, which allows a user to disallow non-confidential and confidential transfers. Closing since it's been nearly a year!