Deploy and proposal for USDT market on Goerli

[cwang25]

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]

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]

 ···································|··············|·················
 |  CometFactory                    ·      24.044  ·                │
 ···································|··············|·················
 |  CometModifiedFactory            ·      24.182  ·                │
 ···································|··············|·················

The contract size is approaching to limit, compiler will complain with Warning: 2 contracts exceed the size limit for mainnet deployment.
But the actual size limit is 24.576 kb, so we should be fine :P

[kevincheng96]

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.

[kevincheng96]

Nice work on the tests! They look great 👍

[kevincheng96]

Think this should be ready for audit with OZ now!

Could you update the PR description to say this introduces a new implementation of Comet that supports non-standard ERC20s and fee tokens?

Separately, I wonder if we should add this logic in a new Comet file, like NonStandardComet or FeeTokenComet?

[kevincheng96]

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().

[cwang25]

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]

Now the accounting in comet won't be inflated by re-entrancy supply reaching SupplyCapExceeded().
It will be blocked and throw TransferInFailed() error.

[cwang25]

Now the accounting in comet won't be mistaken by re-entrancy withdraw reaching NotColalteralized() error.
Instead, it will be blocked and throw TransferInFailed() error right away.

[kevincheng96]

Shouldn't it throw the re-entrancy error instead?

[cwang25]

Yeah it was bit weird, but since the error is thrown in the nested transfer() function, so it gets sort of "re-wrap" into TransferInFailed() as the code here doesn't re-raise the error (or should we make it re-raise the error?)

uint256 preTransferBalance = ERC20(asset).balanceOf(address(this));
        (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount));
        if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) {
            revert TransferInFailed();
        }
        return ERC20(asset).balanceOf(address(this)) - preTransferBalance;

From code we can see now the transferFrom revert won't reveirt right away with returndata, but instead when it fails, we throw TransferInFailed().

[kevincheng96]

Can just inline rather than creating constants. Also, let's use a default value of 0. Otherwise, you need to initialize the slot to 1.

[cwang25]

I recalled seeing in openzeppelin's reentrancy guard contract saying that using 1, 2 is cheaper on running than setting it to 0. (reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/ReentrancyGuard.sol)

Since we're only checking on if status == COMET_REENTRANCY_GUARD_ENTERED. It looks like the initialization value of 0 or 1 wouldn't affect the first run behavior, so I ended up changing to 1,2. And each run it only checks on if the status is ENTERED(2) :P. But yeah I agree the first initialization value having to be 0 instead of 1 is a bit confusing, and I can change it back to 0 if the gas saving gain is not enough to tradeoff with the confusion initialization values.

[kevincheng96]

Having nonReentrant on the transfer functions breaks BuyCollateral right?

[kevincheng96]

Let's add an assertion to assert the re-entrancy error.

[kevincheng96]

Test name should change as well

[kevincheng96]

Shouldn't it throw the re-entrancy error instead?