Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Track ERC20 Allowances #1831

Open
DefiDebauchery opened this issue Jan 16, 2024 · 3 comments
Open

Feature Request: Track ERC20 Allowances #1831

DefiDebauchery opened this issue Jan 16, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@DefiDebauchery
Copy link

DefiDebauchery commented Jan 16, 2024

What is needed?

Since the TXS is already tracking token spending (transfer), indexing Approvals and reconciling them with subsequent Transfers would be useful.

Background

The Bungee security issue on Jan 16th caused us at Beefy to scramble to determine if any of our Safes were potentially impacted by outstanding approvals. While we generally keep our approvals tied to our expected short-term usage, not everyone might (given the effort required to get signers for repeated transactions). This highlighted a greater desire to keep a better eye on Approval data.

While third-party tools exist to track such data, Safe is nearly halfway there with its token Transfer indexing. By also indexing Approval events and reconciling them with these Transfers, Safe could provide users an in-house, at-a-glance overview of forgotten Approvals, and thus potential security vectors.

Endpoint

I don't think there's any need for a separate endpoint; simply adding a new Approvals (or Spenders?) Model and key, containing address => value pairs within each token in SafeBalanceResponse, would be effective.

  {
    "tokenAddress": "0xc55E93C62874D8100dBd2DfE307EDc1036ad5434",
    "token": {
      // ...
    },
    "balance": "31196877661694",
    "spenders": {
      "0x1111111254EEB25477B68fb85Ed929f73A960582": "1234567890123456780",
      "0x5a32F67C5eD74dc1b2e031b1bc2c3E965073424F": "987654321098765430"
    }
  },

Implementation Details

I'm admittedly not as well-versed in the TXS infrastructure as I'd like, but my high-level thoughts would be the following:

  • Enhance indexers to track token Approval events that match the indexed Safe owner.
  • Capture the associated spender, adding or updating the value as a database entry. Because Safe is already tracking unique transactions, I don't think this table needs to uniquely catalog each balance change, but could exist as a simple key-value pair.
  • During each Transfer event - which the TXS is already tracking - subtract that amount from the spender, much like the smart contract itself would do.

This would pave the way for providing a live view of approved spenders and remaining allowance within the API, and thus the UI. This may require that the output of /v1/.../balances report tokens with zero balance if there are non-zero Approvals.

Challenges

  • Building out existing approvals is a bit of a daunting task, especially for networks with a long chain history. I imagine Safe has experience with this and knows the best option for doing so. I don't think this alone should be a blocker.
    • A possible and adequate work-around would be to adjust the Transfer event logic in such a way that if an existing Approval record doesn't exist, query it from the token contract and store it at that time.
@DefiDebauchery DefiDebauchery added the enhancement New feature or request label Jan 16, 2024
@Uxio0
Copy link
Member

Uxio0 commented Feb 6, 2024

@DefiDebauchery thanks for the really well documented issue.

I understand your concern and motivation for the issue, but I don't think we are implementing it:

  • There are other services doing that already. In the tx service we try to focus on things related to the Safe and provide information no one else is providing. Token indexing is something we started doing in the past when it was not so easy to get a portfolio about transfers in different chains and might be removed in the future so we can focus in Safe related logics, as there are a lot of indexers/block explorers/portfolio managers that provide token related information.
  • More resources spent and costs for indexing allowances.
  • A lot of work, we are currently focusing on 4337 Account Abstraction.

Let me know if my reasoning looks good to you, and thanks again for contributing.

@DefiDebauchery
Copy link
Author

It's true that other services track allowances, but just like the portfolio services you mention, they often work on a whitelist, and can sometimes miss tokens that we (or any Safe owner) would care about.

I know there's a ton of token spam, but I do hope Safe does not decide to abandon token indexing. For Beefy specifically, where we are managing 18+ Safes all on different chains, having that de facto source of truth through the API is fairly critical; we use this data to power internal tooling, and we'd similarly use Allowance data.

In the end, I understand that it's a big ask, especially in the midst of preparing for 4337. I do hope that Safe will at least hold this open for evaluation after those higher priorities have been tackled.

@Uxio0
Copy link
Member

Uxio0 commented Feb 16, 2024

Wouldn't you be able to switch to another source of truth like a blockexplorer or a specialized solution like Zerion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants