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

Add assumptions about decimals implementation #337

Closed
smol-ninja opened this issue Nov 12, 2024 · 5 comments
Closed

Add assumptions about decimals implementation #337

smol-ninja opened this issue Nov 12, 2024 · 5 comments
Labels
effort: low Easy or tiny task that takes less than a day. priority: 1 This is important. It should be dealt with shortly. type: docs Changes to documentation. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.

Comments

@smol-ninja
Copy link
Member

smol-ninja commented Nov 12, 2024

In Security.md, add the following assumptions:

- The token contract must implement `decimals()` in order to be compatible with Flow.
- The `decimals` must remain immutable and must not be altered.
@smol-ninja smol-ninja added priority: 1 This is important. It should be dealt with shortly. effort: low Easy or tiny task that takes less than a day. type: docs Changes to documentation. work: clear Sense-categorize-respond. The relationship between cause and effect is clear. labels Nov 12, 2024
@smol-ninja smol-ninja changed the title Add assumptions about decimals Add assumptions about decimals implementation Nov 12, 2024
@PaulRBerg
Copy link
Member

PaulRBerg commented Nov 12, 2024

Yeah we could do this but lacking a decimals value is so bananas that I feel like this assumption is part of 99% of DeFi protocols.

@smol-ninja
Copy link
Member Author

smol-ninja commented Nov 13, 2024

this assumption is part of 99% of DeFi protocols

Not true. Most protocols do not rely on decimals at the protocol level. As examples (shared by an auditor), the following two token can be traded on Uniswap / Sushiswap but does not have decimals implemented:

  1. https://etherscan.io/token/0xe0b7927c4af23765cb51314a0e0521a9645f0e2a#tokenTrade
  2. https://etherscan.io/token/0x1da4858ad385cc377165a298cc2ce3fce0c5fd31#tokenTrade

On the contrary, the percentage of protocols that rely on decimals being implemented may be smaller than we are anticipating. DEXes definitely don't require to have decimals implemented since their price formula simply takes the ratio of the two in a pool, lending protocols don't seem to require decimals as well since they rely on oracles and these two probably make the most of the DeFi today.

@PaulRBerg
Copy link
Member

I was referring to protocols that do rely on decimals, they assume that it exists. It's really bananas. But yes, anyway, let's be explicit about this and mention it in SECURITY.

@smol-ninja
Copy link
Member Author

Agree its bananas. This will still be mentioned as a low finding.

@smol-ninja
Copy link
Member Author

Resolved in 378049e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: low Easy or tiny task that takes less than a day. priority: 1 This is important. It should be dealt with shortly. type: docs Changes to documentation. work: clear Sense-categorize-respond. The relationship between cause and effect is clear.
Projects
None yet
Development

No branches or pull requests

2 participants