Closed rndquu closed 7 months ago
All right! Let's get to work.
it's work time!
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.
The current list is as follows:
https://github.com/ubiquity/ubiquity-dollar/issues/867 https://github.com/ubiquity/ubiquity-dollar/issues/868 https://github.com/ubiquity/ubiquity-dollar/issues/869 https://github.com/ubiquity/ubiquity-dollar/issues/870 https://github.com/ubiquity/ubiquity-dollar/issues/871 https://github.com/ubiquity/ubiquity-dollar/issues/872 https://github.com/ubiquity/ubiquity-dollar/issues/873
Now the only remaining is to tick the check boxes after fixing.. -)
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.
i like the idea of this AI naming issues, i begin with one 😄
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 intodevelopment
branch later.
No, instead we need to accumulate the PR fixes first, then submit that on the audit repo
! Pricing is disabled on parent issues.
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 intodevelopment
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!
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 intodevelopment
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
.
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 intodevelopment
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.
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 intodevelopment
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 intofix-review
when the issue is considered closed as completed? Once all the issues are closed as completed, then we mergefix-review
intodevelopment
?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.
@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:
get_dy()
returns <1 when liquidity <1)CRV
which is >1$ due to accrued fees)+1 let's use newer curve metapool as there probably more bug fixes underneath
+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.
+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 @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
@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.
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?
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.
We've made it @pavlovcik @rndquu @gitcoindev
UBQ whole year🥳
! action has an uncaught error
FYI the next steps are:
The list of sherlock's audit issues that we must fix.
Notice:
fix-review
branch (create one if necessary)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