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.

Sign up for GitHub

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

Jump to bottom

Address OZ's feedback on USDT comet support #818

Merged

cwang25 merged 36 commits into hans/goerli-usdt-deploy from hans/goerli-usdt-oz-feedback

Feb 7, 2024

Contributor

cwang25 commented Nov 8, 2023

No description provided.

Hans Wang added 5 commits

October 30, 2023 11:13


          address L-02

369b363


          use ethcall instead of assembly

f7bf99a


          using low level call, so won't need IERC20Nonstandard as oppose for E…

…RC20.


          add reentrancy guard, and updated tests

85be018


          address comments

b2f8d03

cwang25 force-pushed the hans/goerli-usdt-deploy branch from 85be018 to a13050a Compare

November 8, 2023 19:57

Hans Wang added 3 commits

November 8, 2023 12:15


          inline nonReentrant call

2c72dff


          update on tests

499dce8


          intent

3d7fd01

cwang25 commented

View reviewed changes

test/buy-collateral-test.ts Show resolved Hide resolved

cwang25 requested a review from kevincheng96

November 8, 2023 21:25

kevincheng96 reviewed

View reviewed changes

contracts/Comet.sol Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

contracts/Comet.sol Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

contracts/Comet.sol Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

contracts/CometCore.sol Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

contracts/CometCore.sol Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

contracts/IERC20NonStandard.sol Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/buy-collateral-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/supply-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/supply-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/supply-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/supply-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/supply-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/withdraw-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

test/withdraw-test.ts Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

contracts/Comet.sol Outdated Show resolved Hide resolved

kevincheng96 reviewed

View reviewed changes

contracts/Comet.sol Outdated Show resolved Hide resolved

cwang25 and others added 3 commits

November 8, 2023 18:45


          Update test/buy-collateral-test.ts

79ac88a

Co-authored-by: Kevin Cheng <[email protected]>


          Update test/supply-test.ts

5ef9cfd

Co-authored-by: Kevin Cheng <[email protected]>


          Update test/withdraw-test.ts

6a936d1

Co-authored-by: Kevin Cheng <[email protected]>

Hans Wang added 6 commits

November 9, 2023 16:40


          trim down a bit on contract size

8856e82


          fix tests

e6d6a86


          fix lint

9f770b4


          use assembly for transfer action, as it save more spaces, and it's al…

46896c9

…ready in-use in other contracts


          tests update

9a33adc


          Add memory safe assembly

f4df8a2

kevincheng96 reviewed

View reviewed changes

contracts/Comet.sol Show resolved Hide resolved

contracts/Comet.sol Outdated Show resolved Hide resolved

contracts/CometCore.sol Outdated Show resolved Hide resolved

contracts/CometCore.sol Outdated Show resolved Hide resolved

Hans Wang added 3 commits

November 28, 2023 15:05


          change reentrant to separate functions to reduce space

d688672


          address comments

99da5c4


          add memory-safe as recommended in other palces

22830d0

cwang25 requested a review from kevincheng96

November 28, 2023 23:34

vincetiu8 mentioned this pull request

Add USDT market on ETH mainnet #827

Open

kevincheng96 approved these changes

View reviewed changes

Contributor

kevincheng96 left a comment

Nice, just a few final comments. But this is looking almost ready now 👍

contracts/Comet.sol Show resolved Hide resolved

test/buy-collateral-test.ts Outdated Show resolved Hide resolved

Hans Wang added 2 commits

February 5, 2024 17:19


          address comments

53376ed


          lint

58d1df6

kevincheng96 reviewed

View reviewed changes

contracts/Comet.sol Outdated Show resolved Hide resolved

contracts/Comet.sol Outdated Show resolved Hide resolved

contracts/Comet.sol Outdated Show resolved Hide resolved

test/buy-collateral-test.ts Outdated Show resolved Hide resolved

test/buy-collateral-test.ts Outdated

@@ @@ -562,5 +562,68 @@ describe('buyCollateral', function () { @@
                     // EvilToken attack should be blocked, so totalSupplyBase should be 0
                     expect(evilBobPortfolio.internal.EVIL).to.equal(0);
                   });
+                  it('reentrant supply is reverted', async () => {

Contributor

kevincheng96 Feb 6, 2024

Should be buycollateral, not supply. same for comments in test

Contributor Author

cwang25 Feb 6, 2024

Ok renamed to buyCollateral. :)
Thought in the existing test is using 'supply', and decided to name it the same.
(I guess that rationale with that was the retrant attack loop is happening in supplyFrom via buyCollateral)

Contributor

kevincheng96 Feb 6, 2024

Oh I see. That means the test isn't testing the right function. It should be triggering reentrancy via BuyCollateral. I think you'll have to set it in the EvilToken.

cwang25 and others added 7 commits

February 5, 2024 18:56


          Update contracts/Comet.sol

7d9dbe5

Co-authored-by: Kevin Cheng <[email protected]>


          Update contracts/Comet.sol

d45e8fe

Co-authored-by: Kevin Cheng <[email protected]>


          Update contracts/Comet.sol

b53c096

Co-authored-by: Kevin Cheng <[email protected]>


          Update test/buy-collateral-test.ts

5c10dab

Co-authored-by: Kevin Cheng <[email protected]>


          address more comments

d077e35


          address comment to let new test to test on buyCollateral attacks spec…

23cb021

…ifically


          evil token update

dd10703

kevincheng96 reviewed

View reviewed changes

deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts Outdated Show resolved Hide resolved

contracts/test/EvilToken.sol Outdated Show resolved Hide resolved

Hans Wang added 2 commits

February 6, 2024 17:00


          remove accidently checked in test file

763f806


          fix last two comments

68204da

kevincheng96 approved these changes

View reviewed changes

Contributor

kevincheng96 left a comment

Great, looks ready to merge

cwang25 merged commit 513f451 into hans/goerli-usdt-deploy

18 of 34 checks passed

vincetiu8 pushed a commit to vincetiu8/comet that referenced this pull request


          Address OZ's feedback on USDT comet support (compound-finance#818)

b5ee2e5

Address oz's feedback on USDT comet support

vincetiu8 pushed a commit to vincetiu8/comet that referenced this pull request


          Address OZ's feedback on USDT comet support (compound-finance#818)

35b3038

Address oz's feedback on USDT comet support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet