Conquer the Enigma of Random Crashes: Audit - must fix

[rndquu]

The list of sherlock's audit issues that we must fix.

Notice:

1. All PRs with fixes should be opened towards the fix-review branch (create one if necessary)
2. PR links should be posted in sherlock's github audit repo

• cergyk - LibTWAPOracle::update temporary redemption DOS if uAD/3CRV metapool has very low liquidity sherlock-audit/2023-12-ubiquity-judging#105
• rvierdiiev - AccessControlFacet doesn't have ability to set admin for the role sherlock-audit/2023-12-ubiquity-judging#90
• cergyk - UbiquityPool::redeemDollar DoS on redeeming if USDT/USDC depegs sherlock-audit/2023-12-ubiquity-judging#59
• cergyk - LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs sherlock-audit/2023-12-ubiquity-judging#56
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on arbitrarily short TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#20
• cergyk - LibTWAPOracle::setPool Dos on setPool by donating a few wei of 3CRV directly to the metapool sherlock-audit/2023-12-ubiquity-judging#14
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on outdated TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#13
• 0xAlix2 - AmoMinter can borrow collateral more than what's free/available sherlock-audit/2023-12-ubiquity-judging#1
• Add view method for redeemCollateralBalances (required for ubiquity pool UI, sherlock team accepted this change but we need to show the related PR to our lead senior watson)

P.S. The list may be updated
P.P.S. Pls mention in the comments if you're working on some of the above issues so that we don't do the same job

@gitcoindev @molecula451 Help is welcome

[gitcoindev]

All right! Let's get to work.

[molecula451]

it's work time!

[molecula451]

#867 still up! until mention otherwise

[gitcoindev]

I will create issues in this repo referencing the ones from the audit. This will improve visibility and we will be able to self-assign and start working without clashes.

[gitcoindev]

The current list is as follows:

#867
#868
#869
#870
#871
#872
#873

Now the only remaining is to tick the check boxes after fixing.. -)

[gitcoindev]

Branch https://github.com/ubiquity/ubiquity-dollar/tree/fix-review created . Please correct me if I am wrong @rndquu , the fixes must target this fix-review branch and will be merged into development branch later.

[molecula451]

i like the idea of this AI naming issues, i begin with one 😄

[molecula451]

Branch https://github.com/ubiquity/ubiquity-dollar/tree/fix-review created . Please correct me if I am wrong @rndquu , the fixes must target this fix-review branch and will be merged into development branch later.

No, instead we need to accumulate the PR fixes first, then submit that on the audit repo

[ubiquibot]

! Pricing is disabled on parent issues.

[gitcoindev]

Branch https://github.com/ubiquity/ubiquity-dollar/tree/fix-review created . Please correct me if I am wrong @rndquu , the fixes must target this fix-review branch and will be merged into development branch later.

No, instead we need to accumulate the PR fixes first, then submit that on the audit repo

Ok got it, thank you for clarifying!

[rndquu]

Branch https://github.com/ubiquity/ubiquity-dollar/tree/fix-review created . Please correct me if I am wrong @rndquu , the fixes must target this fix-review branch and will be merged into development branch later.

Yes, all PR fixes must target the fix-review branch. Once PR is merged it's link should be posted in sherlock's audit repo in the original audit issue's comments. The fix-review branch will be merged into development.

[0x4007]

Branch https://github.com/ubiquity/ubiquity-dollar/tree/fix-review created . Please correct me if I am wrong @rndquu , the fixes must target this fix-review branch and will be merged into development branch later.

Yes, all PR fixes must target the fix-review branch.

I assume the plan is to use fix-review as our trunk (or starting/root branch) and then branch off to handle each issue, and then merge back into fix-review when the issue is considered closed as completed? Once all the issues are closed as completed, then we merge fix-review into development?

If that is the correct understanding then I think its a fine strategy.

[rndquu]

Branch https://github.com/ubiquity/ubiquity-dollar/tree/fix-review created . Please correct me if I am wrong @rndquu , the fixes must target this fix-review branch and will be merged into development branch later.

Yes, all PR fixes must target the fix-review branch.

I assume the plan is to use fix-review as our trunk (or starting/root branch) and then branch off to handle each issue, and then merge back into fix-review when the issue is considered closed as completed? Once all the issues are closed as completed, then we merge fix-review into development?

If that is the correct understanding then I think its a fine strategy.

Yes, this is the strategy described by sherlock as the best practise.

[rndquu]

@molecula451 @gitcoindev

The latest curve's metapool implementation has built-in TWAP and adjustable time window (more info in this issue) so I think we can solve all TWAP related issues by simply utilizing the latest curve's metapool contract.

Pls don't start the following issues while I'm carrying out a research:

• cergyk - LibTWAPOracle::update temporary redemption DOS if uAD/3CRV metapool has very low liquidity sherlock-audit/2023-12-ubiquity-judging#105 (get_dy() returns <1 when liquidity <1)
• cergyk - UbiquityPool::redeemDollar DoS on redeeming if USDT/USDC depegs sherlock-audit/2023-12-ubiquity-judging#59 (Dollar is tied to CRV which is >1$ due to accrued fees)
• cergyk - LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs sherlock-audit/2023-12-ubiquity-judging#56 (use accumulated spot prices instead of balances)
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on arbitrarily short TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#20 (increase time window sample)
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on outdated TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#13 (update underlying metapool's TWAP on mint/redeem)

[molecula451]

+1 let's use newer curve metapool as there probably more bug fixes underneath

[gitcoindev]

+1 let's use newer curve metapool as there probably more bug fixes underneath

Yes, this looks reasonable and hopefully would allow to kill fix many birds bugs with one stone change.

[0x4007]

+1 let's use newer curve metapool as there probably more bug fixes underneath

Yes, this looks reasonable and hopefully would allow to kill fix many birds bugs with one stone change.

lol

[molecula451]

@molecula451 @gitcoindev

The latest curve's metapool implementation has built-in TWAP and adjustable time window (more info in this issue) so I think we can solve all TWAP related issues by simply utilizing the latest curve's metapool contract.

Pls don't start the following issues while I'm carrying out a research:

• cergyk - LibTWAPOracle::update temporary redemption DOS if uAD/3CRV metapool has very low liquidity sherlock-audit/2023-12-ubiquity-judging#105 (get_dy() returns <1 when liquidity <1)
• cergyk - UbiquityPool::redeemDollar DoS on redeeming if USDT/USDC depegs sherlock-audit/2023-12-ubiquity-judging#59 (Dollar is tied to CRV which is >1$ due to accrued fees)
• cergyk - LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs sherlock-audit/2023-12-ubiquity-judging#56 (use accumulated spot prices instead of balances)
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on arbitrarily short TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#20 (increase time window sample)
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on outdated TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#13 (update underlying metapool's TWAP on mint/redeem)

any update on the issues research, @rndquu

[rndquu]

@molecula451 @gitcoindev
The latest curve's metapool implementation has built-in TWAP and adjustable time window (more info in this issue) so I think we can solve all TWAP related issues by simply utilizing the latest curve's metapool contract.
Pls don't start the following issues while I'm carrying out a research:

• cergyk - LibTWAPOracle::update temporary redemption DOS if uAD/3CRV metapool has very low liquidity sherlock-audit/2023-12-ubiquity-judging#105 (get_dy() returns <1 when liquidity <1)
• cergyk - UbiquityPool::redeemDollar DoS on redeeming if USDT/USDC depegs sherlock-audit/2023-12-ubiquity-judging#59 (Dollar is tied to CRV which is >1$ due to accrued fees)
• cergyk - LibTWAPOracle::update Providing large liquidity will manipulate TWAP, DOSing redeem of uADs sherlock-audit/2023-12-ubiquity-judging#56 (use accumulated spot prices instead of balances)
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on arbitrarily short TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#20 (increase time window sample)
• cergyk - LibUbiquityPool::mintDollar/redeemDollar reliance on outdated TWAP oracle may be inefficient for preventing depeg sherlock-audit/2023-12-ubiquity-judging#13 (update underlying metapool's TWAP on mint/redeem)

any update on the issues research, @rndquu

The latest curve's metapool implementation has a built-in TWAP oracle which solves most of the TWAP issues found by Sherlock's watsons. Working on a PR that removes our own TWAP implementation and uses the TWAP from curve's metapool.

[gitcoindev]

I suppose we can close this issue. I will still go through remaining minor issues but the 'must fix' list seems to have been completed. @rndquu , @molecula451 could you please confirm?

[rndquu]

I suppose we can close this issue. I will still go through remaining minor issues but the 'must fix' list seems to have been completed. @rndquu , @molecula451 could you please confirm?

Right now there is the "fix review" stage when lead senior watson checks the fixes. I think it makes sense to close this one when LSW confirms that everything is fine.

[molecula451]

We've made it @pavlovcik @rndquu @gitcoindev

UBQ whole year🥳

[ubiquibot]

! action has an uncaught error

[rndquu]

FYI the next steps are:

1. I will update chore: UbiquityPool UI Update #860 to work with the updates introduced in the PRs related to the audit
2. I will prepare the deployment script