Collateral TWAP price miscalculation

[rndquu]

We have the UbiquityPoolFacet contract (which uses LibUbiquityPool library under the hood) which is responsible for minting/redeeming Dollar tokens for collateral.

Example:

1. User calls mintDollar() and sends 100 LUSD as collateral
2. User gets 100 Dollars

Now check this test. It basically simulates the calcMintDollarAmount() method responsible for calculating how much Dollars should be minted/redeemed in exchange for collateral tokens.

The calculation seems to be wrong because when we try to get the collateral price in USD here

ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol

Line 438 in 7a70182

.get_price_cumulative_last()[0];

we use cumulative price which seems to be wrong. As a result user gets much more Dollars in exchange for collateral. Example:

1. User calls mintDollar() and sends 100 LUSD as collateral
2. User gets ~239561778028138198.47 Dollars which is way too high

As a part of the current issue we should add a new method to the oracle faucet which will be responsible for fetching collateral token prices in USD.

Possible options:

1. Curve TWAP (we already use it for the Dollar token)
2. Chainlink oracles

[ubiquibot]

You have been unassigned from the bounty @pavlovcik

[rndquu]

is this open for attempting @rndquu

Hey, yes

[gitcoindev]

No progress was made, I will try to find the root cause and hopefully fix this.

[gitcoindev]

I think I found a way to implement this correctly.

Running 1 test for test/diamond/facets/UbiquityPoolFacet.t.sol:UbiquityPoolFacetTest
[PASS] testCollateralTwap() (gas: 133837)
Logs:
  LUSD Curve MetaPool tokens
  0 (LUSD): 0x5f98805A4E8be255a32880FDeC7F6728C6568bA0
  1 (3CRV): 0x6c3F90f043a72FA612cbac8115EE7e52BDe6E490
  get_price_cumulative_last()[0] (LUSD) : 2576835979968650582055295336346502
  get_price_cumulative_last()[1] (3CRV) : 4894835369996648397661910717659729
  block_timestamp_last: 1700776559
  dy last: 967501938862099827
  dollarsOut: 90388445519065770216

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.64s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

...

Running 1 test for test/diamond/facets/UbiquityPoolFacet.t.sol:UbiquityPoolFacetTest
[PASS] testCollateralTwap() (gas: 133837)
Logs:
  LUSD Curve MetaPool tokens
  0 (LUSD): 0x5f98805A4E8be255a32880FDeC7F6728C6568bA0
  1 (3CRV): 0x6c3F90f043a72FA612cbac8115EE7e52BDe6E490
  get_price_cumulative_last()[0] (LUSD) : 2576865692445777709373565553899126
  get_price_cumulative_last()[1] (3CRV) : 4894843303717373371511942034905809
  block_timestamp_last: 1700778623
  dy last: 967448572577455653
  dollarsOut: 90383459797261648426

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.51s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Last available price gives for 100 LUSD collateral about 90.388 3CRV tokens which seems correct when fee is included. I need to think over how to implement a test. likely I will create LUSD Curve MetaPool mock with values, so that forking will not be needed in forge. From balances and time delta we will get VWAP price using two pairs of get_price_cumulative_last and elapsed time. I will describe the calculations in a detail and open a PR tomorrow morning.

[ubiquibot]

Do you have any updates @gitcoindev? If you would like to release the bounty back to the DevPool, please comment /stop
Last activity time: Thu Nov 23 2023 22:36:21 GMT+0000 (Coordinated Universal Time)

[gitcoindev]

I will try to finish by tomorrow EOD, just wrapping up an another PR.

[gitcoindev]

In the end I am going to implement also chainlink oracle support for any collateral in scope of this ticket which is half baked already on my local set up.

[rndquu]

In the end I am going to implement also chainlink oracle support for any collateral in scope of this ticket which is half baked already on my local set up.

Yes, there is no need to overengineer the solution, chainlink is fine.

As a part of the current issue we were expecting:

1. Implement chainlink oracles for collateral token prices
2. Check that our current Dollar oracle works as expected (and can't be easily manipulated, but it depends on the curve metapool liquidity, as far as I understand)

[gitcoindev]

I will try to finalize this tomorrow till EOD.

[ubiquibot]

@@ Need to reimplement penalty system @@

[ubiquibot]

+ Evaluating results. Please wait...

[ubiquibot]

@@ Need to reimplement penalty system @@

[ubiquibot]

+ Evaluating results. Please wait...

[ubiquibot]

@@ Need to reimplement penalty system @@

[ubiquibot]

+ Evaluating results. Please wait...

[molecula451]

@pavlovcik i thoght we'd nailed the private key bug?

[0x4007]

I'm not sure. To be honest I never felt mastery around the X25519 key related feature so it's possible that I'm making a mistake in the configuration.

[rndquu]

@pavlovcik

You can see in the logs that the bot reads evmPrivateEncrypted: 'OwWrEtDWl7mq79FM-mb4hbrU9uBtffvX3MPuEjcXenWocH-JS4SVYXFr0rm-wYuWHnwrzLYvatKGywShwMVCEZw6f-g2XyDIPdrEzAHCZ8WH_v76kJsVEOGEe8qRvy4z6ai0YGFpzV4iFB8NfZEg5kb9k88', just fine.

I've just successfully decrypted this private key for the 0xf87ca4583c792212e52720d127e7e0a38b818ad1 address. I will DM you x25519_private_key and x25519_public_key for you to double check.

I understand that the decryption failed due to a mismatched X25519_PRIVATE_KEY?

Yes

I just checked the source code (we obviously need a more secure replacement for this onboarding UI.) and used the identical X25519_PRIVATE_KEY but the run still failed.

This is public key, not private, so it is safe to share it. Naming could've been better.

Is this approach okay, where the bot can read the evmPrivateEncrypted of every partner, and we simply store a single X25519_PRIVATE_KEY in the bot secrets?

Yes. The only risk is that if X25519_PRIVATE_KEY is leaked (on our side) then a malicious actor will be able to decrypt partners' private keys (if there are in public repos, not private).

If any partner can encrypt their EVM private key, doesn't that mean they necessarily have access to our source X25519_PRIVATE_KEY?

No. Partners encrypt with X25519_PUBLIC_KEY (which is available in public) while for decrypting X25519_PRIVATE_KEY is necessary which is stored only on the bot's side (i.e. backend).

Are there any improvements you can suggest?

You've already proposed that we need critical env variables rotation mechanism and X25519_PRIVATE_KEY is the 1st candidate for such rotation.

[0x4007]

Sorry for clouding the conversation on this with debugging the bot!

@rndquu @gitcoindev @molecula451 I have concerns around using Chainlink and censorship resistance. Chainlink is a centralized point of failure for the protocol. Why not use the on chain TWAPs instead? Perhaps we can have a planned upgrade sometime in late 2024 to change the oracle(s)?

[molecula451]

Indeed a well thought chainlink it's a heavily centrilized organization which the only good thing they provide it's infrastructure as a service and well known set of poketing of smart contracts, and use cases in with heavily demanded protocols that are using it, but that's it

[gitcoindev]

ChainLink is an industry standard but yes, this should not be a single point of failure.

When I started working on TWAP I realized the security implications of using just TWAP, and expected some findings from the audit as well. We should think this through. Since the contracts were partially based on Frax I had a look at latest oracles there and their implementation is very good in this matter.

https://docs.frax.finance/frax-oracle/frax-oracle-overview
https://docs.frax.finance/frax-oracle/how-it-works
https://docs.frax.finance/frax-oracle/advanced-concepts

The executive summary is as follows: to retrieve frxEth / ETH price they use dual oracles that combine ChainLink and Uniswap V3 TWAP feeds into one feed, provided lowest and highest prices from those two.

In their case for frxEth price is bounded to be 0.7 at the lowest (0.7 frxEth = 1 ETH) and 1 at the highest (1 frxEth = 1 ETH).

On top of that they have implemented a Frax Oracle contract that uses feeds from above dual oracles for different pairs and different networks.

If one of the price providers in dual oracle fails / is stalled or censored, the second price will be used. Perhaps some blogs or articles explain this in more detail.

The oracles implementation repository was created around 4 months ago https://github.com/FraxFinance/frax-oracles

I think it also might be beneficial to dig into Sherlock past contests to see if they already audited those oracles or some other projects that use them and the findings they had. If no such audits are available we could as well try to find other approaches for oracles from past security audits and try to create an overview of the available most secure solutions. Then pick one and upgrade the current solution.

[rndquu]

Sorry for clouding the conversation on this with debugging the bot!

@rndquu @gitcoindev @molecula451 I have concerns around using Chainlink and censorship resistance. Chainlink is a centralized point of failure for the protocol. Why not use the on chain TWAPs instead? Perhaps we can have a planned upgrade sometime in late 2024 to change the oracle(s)?

We'll gradually move to smth like "take the median price from chainlink, curve, uniswap, other oracle provider". Right now chainlink is the fastest to implement, safest and most reliable solution.

[ubiquibot]

+ Evaluating results. Please wait...

[ubiquibot]

+ Evaluating results. Please wait...

[0x4007]

Hey guys, as a workaround I salvaged these permits from the GitHub Actions logs. Sadly its cut off and I couldn't figure out how to see more from the run (sorry @rndquu):

[ 86 WXDAI ]

@pavlovcik

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	11	84.3
Review	Comment	1	1.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
When I get a chance to, I need to add compute.yml to our ubiquib...	3.1	-	3.1
Let\'s see if this works......	0.6	-	0.6
I\'ll debug it here....	0.5	-	0.5
There\'s something wrong with the Netlify environment variables t...	1	-	1
lol also a bug regarding qualitative analysis: mess...</a></h6></td><td><details><summary>6.5</summary> code: count: 1 score: "1" words: 0	-	6.5
Cool, I just need to push a fix regarding the Open AI token limi...	1.4	-	1.4
Just set context length to 128k......	0.6	-	0.6
Can\'t get OpenAI to cooperate anymore....	0.7	-	0.7
I coerced the OpenAI relevance sampler to work, but, @rndquu per...	61.2 h3: count: 3 score: "3" words: 3 a: count: 4 score: "4" words: 13 li: count: 11 score: "11" words: 253 code: count: 10 score: "10" words: 14	-	61.2
I\'m not sure. To be honest I never felt mastery around the X2551...	3	-	3
Sorry for clouding the conversation on this with debugging the b...	5.7	-	5.7
Curve metapool seems to make the most sense to me. Can also cons...	1.7	-	1.7

[ 16.8 WXDAI ]

@molecula451

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	2	5.9
Review	Comment	5	10.9

Conversation Incentives

Comment	Formatting	Relevance	Reward
@pavlovcik i thoght we\'d nailed the private key bug?...	1	-	1
Indeed a well thought chainlink it\'s a heavily centrilized organ...	4.9	-	4.9
> I am finally back to this issue, in the background I spent qui...	0.7	-	0.7
@rndquu are you sure? i\'m seeing a deployed chainlink LUSD/USD o...	2.2	-	2.2
> > @rndquu are you sure? i\'m seeing a deployed chainlink LUSD/U...	3	-	3
https://github.com/aave/gho-aip/blob/90ddf49b65007da9ba6dbf9f80c...	1.4	-	1.4
> Yeah, I see that\'s a live `LUSD/USD` oracle but I can\'t unders...	3.6 code: count: 1 score: "1" words: 2	-	3.6

[ 554.1 WXDAI ]

@gitcoindev

Contributions Overview

View	Contribution	Count	Reward
Issue	Task	1.00	300
Issue	Comment	8	0
Issue	Comment	8	90.5
Review	Comment	15	81.8
Review	Comment	15	81.8

Conversation Incentives

Comment	Formatting	Relevance	Reward
No progress was made, I will try to find the root cause and hope...	-	-	-
I think I found a way to implement this correctly. Runni...	- code: count: 2 score: "0" words: 0	-	-