comet icon indicating copy to clipboard operation
comet copied to clipboard

Deploy and proposal for USDT market on Goerli

Open cwang25 opened this issue 2 years ago • 4 comments

This PR introduces the deployment for USDT Goerli as well as the proposal to initialize the market there. For ease of managing the changes the initialize proposal and migration scripts changes are in: PR#802, which will be merged later on.

Comet Contract changes in this PR:

  • Update doTransferIn and doTransferOut to support non-standard erc20,
  • Update doTransferIn to return actual transferred amount for better accounting in non-standard erc20 and fee token scenarios
  • Update supplyBase, supplyCollateral, buyCollateral to account the actual transferred values returned by doTransferIn

TODO:

  • [ ] deploy USDT instance to Goerli
  • [ ] run market initialization proposal

cwang25 avatar Aug 03 '23 00:08 cwang25

The scenario will fail as we encountered back in Arbitrum native usdc deploy. Since this is deploying another comet on the chain with existing governor contract, without proper setFactory, setConfigurator on migration script, the scenario tests will run into errors.

But this can be confirmed in migration PR, which with migration script the scenario run will succeed.

cwang25 avatar Aug 03 '23 18:08 cwang25

Looks great! Could you also add unit tests for supply base, supply collateral, and buy collateral? I notice there is no scenarios for buy collateral, so would be good to get some unit test coverage at least.

I have pushed the unit test for all 3 cases :)

cwang25 avatar Sep 11 '23 23:09 cwang25

Looking at the re-entrancy test fixes, I think the better fix is actually just to have the transfer and transferFrom functions in the EvilToken do actual transfer accounting while also calling performAttack().

kevincheng96 avatar Sep 13 '23 16:09 kevincheng96

Synced with ⛑️ offline. For the re-entrancy issue that Comet is not immune to the tokens that's got exploited. (i.e. EvilToken.sol) We decided to not adding additional layer to prevent that in Comet as this vulnerability will require governance to be exploited that governance has to vote and add the suspicious token into Comet's contract. If suspicious token contract is not added to Comet's market, this should not be a concern.

For reference, if import re-entrancy guard from openzeppelin which can prevent exploited token contract to execute re-entrancy attack, but it will also increase the contract size to:

 ···································|··············|·················
 |  CometFactory                    ·      24.173  ·                │
 ···································|··············|·················
 |  CometModifiedFactory            ·      24.311  ·                │
 ···································|··············|·················

cwang25 avatar Sep 15 '23 00:09 cwang25