[RFP12] CToken Cleanup

[arjun-io]

This PR Implements RFP 12:

Changes

• Upgrades all Contracts to 0.8.10 and removes legacy contracts
• Removes SafeMath and error codes from CToken and Interest Rate contracts, in favor of checked math and custom errors added to Solidity 0.8.x
• Changes to Scenario tests which involve explicit behavior changes due to changing to reverts instead returning

Change Notes

Since Saddle and solc don't support multiple versions within a repo, we thought the best way forward was to upgrade all contracts and remove legacy Comptroller implementations (ComptrollerGN). In order to ensure tests still work, legacy build artifacts were added to tests/LegacyBuilds/legacy.json for usage in tests. We're open to a cleaner solution to this issue - perhaps by supporting multiple solidity versions within a single repo through Hardhat.

We introduce a new CustomError matcher in both regular JS tests and Scenarios - these work by comparing the ABI encoding of the custom error and the results field of the transaction output.

Most math errors with explicit messages (addition overflow, subtraction underflow, divide by zero, etc) have been removed and now just have a generic revert message. This is how Solidity exposes these reverts with checked math, and generally these errors can be debugged using more modern transaction tracing tooling.

[jflatow]

Cool! So what's the gas impact looking like?

[arjun-io]

@jflatow Just ran the profiler and pushed, modest savings. We can save a bit more doing another pass at the optimizations with more code changes (found that removing the localvar structs did save some memory) but wanted to get this version out first

edit: Just removed the LocalVar structs, a bit more savings

[TylerEther]

Should be newProposal.id = proposalId to avoid discrepancies

[TylerEther]

This comment applies to Governor Bravo as well.

[TylerEther]

Let's add a revert below this line if newProposal has any data set. This should never be the case, but if so, we don't want to overwrite anything.

[TylerEther]

This comment applies to Governor Bravo as well.

[jflatow]

I wonder what broke in linting? But I guess this is the future and we need to merge 💪

[arjun-io]

@jflatow I think the current linter (solium) doesn't support newer syntax, it might make sense to switch to solhint

[arjun-io]

@jflatow migrated to solhint. All of the rules mapped pretty cleanly (except indentation):

"quotes" -> "quotes"
"max-len" ->  "max-line-len"
"security/no-block-members" -> "not-rely-on-block-hash" and "not-rely-on-time"
"security/no-inline-assembly" -> "no-inline-assembly"
"security/no-low-level-calls" -> "avoid-low-level-calls"
"security/no-tx-origin" ->. "avoid-tx-origin"
"imports-on-top" -> "imports-on-top"

Hopefully CI will pass now 🙏

[jflatow]

👏

[arjun-io]

@jflatow it seems like the failing tests are due to Circle rather than the tests themselves? What are your thoughts on merging this in and then investigating CI separately (I'm happy to help here)? I know people are developing on top of these changes so it might be nice to have it in master

[enzoferey]

Hi ! Good clean up here 💅🏻

How do you plan to use these changes ? Do you have any intention to audit them ?

[arjun-io]

Hi ! Good clean up here 💅🏻

How do you plan to use these changes ? Do you have any intention to audit them ?

This change has been audited and the cTokens have been upgraded on mainnet. See the forum post for more details