From a669690bfc872d85b7b535b5ef2cfc0d07527620 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Wed, 2 Aug 2023 17:18:18 -0700 Subject: [PATCH 01/31] add usdt deployment --- deployments/goerli/usdt/configuration.json | 55 ++++++++++++++++++++++ deployments/goerli/usdt/deploy.ts | 40 ++++++++++++++++ deployments/goerli/usdt/relations.ts | 12 +++++ hardhat.config.ts | 9 +++- 4 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 deployments/goerli/usdt/configuration.json create mode 100644 deployments/goerli/usdt/deploy.ts create mode 100644 deployments/goerli/usdt/relations.ts diff --git a/deployments/goerli/usdt/configuration.json b/deployments/goerli/usdt/configuration.json new file mode 100644 index 000000000..9159e57d8 --- /dev/null +++ b/deployments/goerli/usdt/configuration.json @@ -0,0 +1,55 @@ +{ + "name": "Compound USDT", + "symbol": "cUSDTv3", + "baseToken": "USDT", + "baseTokenAddress": "0xfad6367E97217cC51b4cd838Cc086831f81d38C2", + "baseTokenPriceFeed": "0xAb5c49580294Aff77670F839ea425f5b78ab3Ae7", + "borrowMin": "10e6", + "governor": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", + "pauseGuardian": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", + "storeFrontPriceFactor": 0.5, + "targetReserves": "5000e18", + "rates": { + "supplyKink": 0.9, + "supplySlopeLow": 0.01690681444, + "supplySlopeHigh": 0.6066567706, + "supplyBase": 0, + "borrowKink": 0.9, + "borrowSlopeLow": 0.05171500002, + "borrowSlopeHigh": 0.5171500339, + "borrowBase": 0.009945209674 + }, + "rewardToken": "COMP", + "tracking": { + "indexScale": "1e15", + "baseSupplySpeed": "447916666666e0", + "baseBorrowSpeed": "0e15", + "baseMinForRewards": "1000e18" + }, + "assets": { + "COMP": { + "priceFeed": "0x54a06047087927D9B0fb21c1cf0ebd792764dDB8", + "decimals": "18", + "borrowCF": 0.65, + "liquidateCF": 0.7, + "liquidationFactor": 0.92, + "supplyCap": "500000e18" + }, + "WBTC": { + "priceFeed": "0xA39434A63A52E749F02807ae27335515BA4b07F7", + "decimals": "8", + "borrowCF": 0.7, + "liquidateCF": 0.75, + "liquidationFactor": 0.93, + "supplyCap": "35000e8" + }, + "WETH": { + "priceFeed": "0xD4a33860578De61DBAbDc8BFdb98FD742fA7028e", + "decimals": "18", + "borrowCF": 0.82, + "liquidateCF": 0.85, + "liquidationFactor": 0.93, + "supplyCap": "1000000e18" + } + } +} \ No newline at end of file diff --git a/deployments/goerli/usdt/deploy.ts b/deployments/goerli/usdt/deploy.ts new file mode 100644 index 000000000..44a7b4acf --- /dev/null +++ b/deployments/goerli/usdt/deploy.ts @@ -0,0 +1,40 @@ +import { Deployed, DeploymentManager } from '../../../plugins/deployment_manager'; +import { debug, DeploySpec, deployComet, exp, sameAddress, wait } from '../../../src/deploy'; + +const clone = { + cbETHImpl: '0x31724cA0C982A31fbb5C57f4217AB585271fc9a5', + cbETHProxy: '0xBe9895146f7AF43049ca1c1AE358B0541Ea49704', +}; + +export default async function deploy(deploymentManager: DeploymentManager, deploySpec: DeploySpec): Promise { + const deployed = await deployContracts(deploymentManager, deploySpec); + return deployed; +} + +async function deployContracts(deploymentManager: DeploymentManager, deploySpec: DeploySpec): Promise { + const ethers = deploymentManager.hre.ethers; + const signer = await deploymentManager.getSigner(); + + // Declare existing assets as aliases + const USDT = await deploymentManager.existing('USDT', '0xfad6367E97217cC51b4cd838Cc086831f81d38C2', 'goerli'); + const COMP = await deploymentManager.existing('COMP', '0x3587b2F7E0E2D6166d6C14230e7Fe160252B0ba4', 'goerli'); + const WBTC = await deploymentManager.existing('WBTC', '0xAAD4992D949f9214458594dF92B44165Fb84dC19', 'goerli'); + const WETH = await deploymentManager.existing('WETH', '0x42a71137C09AE83D8d05974960fd607d40033499', 'goerli'); + + // Import shared contracts from cUSDCv3 + const cometAdmin = await deploymentManager.fromDep('cometAdmin', 'goerli', 'usdc'); + const cometFactory = await deploymentManager.fromDep('cometFactory', 'goerli', 'usdc'); + const $configuratorImpl = await deploymentManager.fromDep('configurator:implementation', 'goerli', 'usdc'); + const configurator = await deploymentManager.fromDep('configurator', 'goerli', 'usdc'); + const rewards = await deploymentManager.fromDep('rewards', 'goerli', 'usdc'); + const fauceteer = await deploymentManager.fromDep('fauceteer', 'goerli', 'usdc'); + const fxRoot = await deploymentManager.fromDep('fxRoot', 'goerli', 'usdc'); + const bulker = await deploymentManager.fromDep('bulker', 'goerli', 'usdc'); + const timelock = await deploymentManager.fromDep('timelock', 'goerli', 'usdc'); + + // Deploy all Comet-related contracts + const deployed = await deployComet(deploymentManager, deploySpec); + const { comet } = deployed; + + return { ...deployed, bulker, fauceteer, fxRoot }; +} \ No newline at end of file diff --git a/deployments/goerli/usdt/relations.ts b/deployments/goerli/usdt/relations.ts new file mode 100644 index 000000000..b757fd1c0 --- /dev/null +++ b/deployments/goerli/usdt/relations.ts @@ -0,0 +1,12 @@ +import baseRelationConfig from '../../relations'; + +export default { + ...baseRelationConfig, + fxRoot: { + relations: { + stateSender: { + field: async fxRoot => fxRoot.stateSender() + } + } + }, +}; diff --git a/hardhat.config.ts b/hardhat.config.ts index ec467b2b7..b9c56ad29 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -20,6 +20,7 @@ import './tasks/scenario/task.ts'; import relationConfigMap from './deployments/relations'; import goerliRelationConfigMap from './deployments/goerli/usdc/relations'; import goerliWethRelationConfigMap from './deployments/goerli/weth/relations'; +import goerliUsdtRelationConfigMap from './deployments/goerli/usdt/relations'; import mumbaiRelationConfigMap from './deployments/mumbai/usdc/relations'; import mainnetRelationConfigMap from './deployments/mainnet/usdc/relations'; import mainnetWethRelationConfigMap from './deployments/mainnet/weth/relations'; @@ -288,7 +289,8 @@ const config: HardhatUserConfig = { networks: { goerli: { usdc: goerliRelationConfigMap, - weth: goerliWethRelationConfigMap + weth: goerliWethRelationConfigMap, + usdt: goerliUsdtRelationConfigMap, }, mumbai: { usdc: mumbaiRelationConfigMap @@ -354,6 +356,11 @@ const config: HardhatUserConfig = { network: 'goerli', deployment: 'weth', }, + { + name: 'goerli-usdt', + network: 'goerli', + deployment: 'usdt', + }, { name: 'mumbai', network: 'mumbai', From 22a4dc83f731f85ef139c3c2ded4ee89e5e3187b Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Wed, 2 Aug 2023 17:20:34 -0700 Subject: [PATCH 02/31] add run scenario entry --- .github/workflows/run-scenarios.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/run-scenarios.yaml b/.github/workflows/run-scenarios.yaml index 65537babf..ee0d544de 100644 --- a/.github/workflows/run-scenarios.yaml +++ b/.github/workflows/run-scenarios.yaml @@ -7,7 +7,7 @@ jobs: strategy: fail-fast: false matrix: - bases: [ development, mainnet, mainnet-weth, goerli, goerli-weth, fuji, mumbai, polygon, arbitrum, arbitrum-goerli-usdc, arbitrum-goerli-usdc.e, base-usdbc, base-weth, base-goerli, base-goerli-weth, linea-goerli] + bases: [ development, mainnet, mainnet-weth, goerli, goerli-weth, goerli-usdt, fuji, mumbai, polygon, arbitrum, arbitrum-goerli-usdc, arbitrum-goerli-usdc.e, base-usdbc, base-weth, base-goerli, base-goerli-weth, linea-goerli] name: Run scenarios env: ETHERSCAN_KEY: ${{ secrets.ETHERSCAN_KEY }} From f0d15032363bb0c26917a622eb38e66eb953c9c0 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 3 Aug 2023 10:21:59 -0700 Subject: [PATCH 03/31] when deploy all suply cap should be zero --- deployments/goerli/usdt/configuration.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/deployments/goerli/usdt/configuration.json b/deployments/goerli/usdt/configuration.json index 9159e57d8..2bbcbe3b5 100644 --- a/deployments/goerli/usdt/configuration.json +++ b/deployments/goerli/usdt/configuration.json @@ -22,9 +22,9 @@ "rewardToken": "COMP", "tracking": { "indexScale": "1e15", - "baseSupplySpeed": "447916666666e0", - "baseBorrowSpeed": "0e15", - "baseMinForRewards": "1000e18" + "baseSupplySpeed": "0.000402083333333e15", + "baseBorrowSpeed": "0.000402083333333e15", + "baseMinForRewards": "10000e6" }, "assets": { "COMP": { @@ -33,7 +33,7 @@ "borrowCF": 0.65, "liquidateCF": 0.7, "liquidationFactor": 0.92, - "supplyCap": "500000e18" + "supplyCap": "0e18" }, "WBTC": { "priceFeed": "0xA39434A63A52E749F02807ae27335515BA4b07F7", @@ -41,7 +41,7 @@ "borrowCF": 0.7, "liquidateCF": 0.75, "liquidationFactor": 0.93, - "supplyCap": "35000e8" + "supplyCap": "0e8" }, "WETH": { "priceFeed": "0xD4a33860578De61DBAbDc8BFdb98FD742fA7028e", @@ -49,7 +49,7 @@ "borrowCF": 0.82, "liquidateCF": 0.85, "liquidationFactor": 0.93, - "supplyCap": "1000000e18" + "supplyCap": "0e18" } } } \ No newline at end of file From 132d591a76d98de454f459e896d4a3de9bd3bc4b Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 3 Aug 2023 10:36:54 -0700 Subject: [PATCH 04/31] use the same usdt contract that compoundV2 is using, so easier to get some usdt --- deployments/goerli/usdt/configuration.json | 2 +- deployments/goerli/usdt/deploy.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/deployments/goerli/usdt/configuration.json b/deployments/goerli/usdt/configuration.json index 2bbcbe3b5..f5f0ec679 100644 --- a/deployments/goerli/usdt/configuration.json +++ b/deployments/goerli/usdt/configuration.json @@ -2,7 +2,7 @@ "name": "Compound USDT", "symbol": "cUSDTv3", "baseToken": "USDT", - "baseTokenAddress": "0xfad6367E97217cC51b4cd838Cc086831f81d38C2", + "baseTokenAddress": "0x79C950C7446B234a6Ad53B908fBF342b01c4d446", "baseTokenPriceFeed": "0xAb5c49580294Aff77670F839ea425f5b78ab3Ae7", "borrowMin": "10e6", "governor": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", diff --git a/deployments/goerli/usdt/deploy.ts b/deployments/goerli/usdt/deploy.ts index 44a7b4acf..337c0fa24 100644 --- a/deployments/goerli/usdt/deploy.ts +++ b/deployments/goerli/usdt/deploy.ts @@ -16,7 +16,7 @@ async function deployContracts(deploymentManager: DeploymentManager, deploySpec: const signer = await deploymentManager.getSigner(); // Declare existing assets as aliases - const USDT = await deploymentManager.existing('USDT', '0xfad6367E97217cC51b4cd838Cc086831f81d38C2', 'goerli'); + const USDT = await deploymentManager.existing('USDT', '0x79C950C7446B234a6Ad53B908fBF342b01c4d446', 'goerli'); const COMP = await deploymentManager.existing('COMP', '0x3587b2F7E0E2D6166d6C14230e7Fe160252B0ba4', 'goerli'); const WBTC = await deploymentManager.existing('WBTC', '0xAAD4992D949f9214458594dF92B44165Fb84dC19', 'goerli'); const WETH = await deploymentManager.existing('WETH', '0x42a71137C09AE83D8d05974960fd607d40033499', 'goerli'); From 83dcc49b79cee7bd9828f2e7f2f9b408b245c032 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 7 Aug 2023 12:52:30 -0700 Subject: [PATCH 05/31] clean up --- deployments/goerli/usdt/deploy.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/deployments/goerli/usdt/deploy.ts b/deployments/goerli/usdt/deploy.ts index 337c0fa24..89c7e19fc 100644 --- a/deployments/goerli/usdt/deploy.ts +++ b/deployments/goerli/usdt/deploy.ts @@ -1,17 +1,7 @@ import { Deployed, DeploymentManager } from '../../../plugins/deployment_manager'; import { debug, DeploySpec, deployComet, exp, sameAddress, wait } from '../../../src/deploy'; -const clone = { - cbETHImpl: '0x31724cA0C982A31fbb5C57f4217AB585271fc9a5', - cbETHProxy: '0xBe9895146f7AF43049ca1c1AE358B0541Ea49704', -}; - export default async function deploy(deploymentManager: DeploymentManager, deploySpec: DeploySpec): Promise { - const deployed = await deployContracts(deploymentManager, deploySpec); - return deployed; -} - -async function deployContracts(deploymentManager: DeploymentManager, deploySpec: DeploySpec): Promise { const ethers = deploymentManager.hre.ethers; const signer = await deploymentManager.getSigner(); @@ -34,7 +24,5 @@ async function deployContracts(deploymentManager: DeploymentManager, deploySpec: // Deploy all Comet-related contracts const deployed = await deployComet(deploymentManager, deploySpec); - const { comet } = deployed; - return { ...deployed, bulker, fauceteer, fxRoot }; } \ No newline at end of file From 6c6e5bffec02ceefa87048efbca91902eb16fa77 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 7 Aug 2023 12:53:06 -0700 Subject: [PATCH 06/31] change targetreserver to 5 usdt --- deployments/goerli/usdt/configuration.json | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/deployments/goerli/usdt/configuration.json b/deployments/goerli/usdt/configuration.json index f5f0ec679..7ae80e0ef 100644 --- a/deployments/goerli/usdt/configuration.json +++ b/deployments/goerli/usdt/configuration.json @@ -8,16 +8,16 @@ "governor": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", "pauseGuardian": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", "storeFrontPriceFactor": 0.5, - "targetReserves": "5000e18", + "targetReserves": "5000000e6", "rates": { - "supplyKink": 0.9, - "supplySlopeLow": 0.01690681444, - "supplySlopeHigh": 0.6066567706, + "supplyKink": 0.8, + "supplySlopeLow": 0.0325, + "supplySlopeHigh": 0.4, "supplyBase": 0, - "borrowKink": 0.9, - "borrowSlopeLow": 0.05171500002, - "borrowSlopeHigh": 0.5171500339, - "borrowBase": 0.009945209674 + "borrowKink": 0.8, + "borrowSlopeLow": 0.035, + "borrowSlopeHigh": 0.25, + "borrowBase": 0.015 }, "rewardToken": "COMP", "tracking": { From e79177585a1c5be225551f16d28a684117b7334c Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 8 Aug 2023 15:31:13 -0700 Subject: [PATCH 07/31] USDT fork from mainnet for better consistent scenario cases --- deployments/goerli/usdt/configuration.json | 1 - deployments/goerli/usdt/deploy.ts | 8 +++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/deployments/goerli/usdt/configuration.json b/deployments/goerli/usdt/configuration.json index 7ae80e0ef..da39f77ec 100644 --- a/deployments/goerli/usdt/configuration.json +++ b/deployments/goerli/usdt/configuration.json @@ -2,7 +2,6 @@ "name": "Compound USDT", "symbol": "cUSDTv3", "baseToken": "USDT", - "baseTokenAddress": "0x79C950C7446B234a6Ad53B908fBF342b01c4d446", "baseTokenPriceFeed": "0xAb5c49580294Aff77670F839ea425f5b78ab3Ae7", "borrowMin": "10e6", "governor": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", diff --git a/deployments/goerli/usdt/deploy.ts b/deployments/goerli/usdt/deploy.ts index 89c7e19fc..de00a82e3 100644 --- a/deployments/goerli/usdt/deploy.ts +++ b/deployments/goerli/usdt/deploy.ts @@ -1,12 +1,18 @@ import { Deployed, DeploymentManager } from '../../../plugins/deployment_manager'; import { debug, DeploySpec, deployComet, exp, sameAddress, wait } from '../../../src/deploy'; +const clone = { + usdt: '0xdAC17F958D2ee523a2206206994597C13D831ec7', +}; + export default async function deploy(deploymentManager: DeploymentManager, deploySpec: DeploySpec): Promise { const ethers = deploymentManager.hre.ethers; const signer = await deploymentManager.getSigner(); + // Clone/fork USDT from mainnet which is non-standard erc20 to testnet + const USDT = await deploymentManager.clone('USDT', clone.usdt, [100_000_000_000_000, 'Tether USD', 'USDT', 6]); + // Declare existing assets as aliases - const USDT = await deploymentManager.existing('USDT', '0x79C950C7446B234a6Ad53B908fBF342b01c4d446', 'goerli'); const COMP = await deploymentManager.existing('COMP', '0x3587b2F7E0E2D6166d6C14230e7Fe160252B0ba4', 'goerli'); const WBTC = await deploymentManager.existing('WBTC', '0xAAD4992D949f9214458594dF92B44165Fb84dC19', 'goerli'); const WETH = await deploymentManager.existing('WETH', '0x42a71137C09AE83D8d05974960fd607d40033499', 'goerli'); From fe550f4e77cdb4926bd782917fff674330abb80a Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 31 Aug 2023 13:18:39 -0700 Subject: [PATCH 08/31] move sol files to deploy branch --- contracts/Comet.sol | 59 +++++++++++++++++++++++++-------- contracts/IERC20NonStandard.sol | 1 + 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 3c5d56442..a368fa732 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.15; import "./CometMainInterface.sol"; -import "./ERC20.sol"; +import "./IERC20NonStandard.sol"; import "./IPriceFeed.sol"; /** @@ -139,7 +139,7 @@ contract Comet is CometMainInterface { **/ constructor(Configuration memory config) { // Sanity checks - uint8 decimals_ = ERC20(config.baseToken).decimals(); + uint8 decimals_ = IERC20NonStandard(config.baseToken).decimals(); if (decimals_ > MAX_BASE_DECIMALS) revert BadDecimals(); if (config.storeFrontPriceFactor > FACTOR_SCALE) revert BadDiscount(); if (config.assetConfigs.length > MAX_ASSETS) revert TooManyAssets(); @@ -241,7 +241,7 @@ contract Comet is CometMainInterface { // Sanity check price feed and asset decimals if (IPriceFeed(priceFeed).decimals() != PRICE_FEED_DECIMALS) revert BadDecimals(); - if (ERC20(asset).decimals() != decimals_) revert BadDecimals(); + if (IERC20NonStandard(asset).decimals() != decimals_) revert BadDecimals(); // Ensure collateral factors are within range if (assetConfig.borrowCollateralFactor >= assetConfig.liquidateCollateralFactor) revert BorrowCFTooLarge(); @@ -482,7 +482,7 @@ contract Comet is CometMainInterface { * @param asset The collateral asset */ function getCollateralReserves(address asset) override public view returns (uint) { - return ERC20(asset).balanceOf(address(this)) - totalsCollateral[asset].totalSupplyAsset; + return IERC20NonStandard(asset).balanceOf(address(this)) - totalsCollateral[asset].totalSupplyAsset; } /** @@ -490,7 +490,7 @@ contract Comet is CometMainInterface { */ function getReserves() override public view returns (int) { (uint64 baseSupplyIndex_, uint64 baseBorrowIndex_) = accruedInterestIndices(getNowInternal() - lastAccrualTime); - uint balance = ERC20(baseToken).balanceOf(address(this)); + uint balance = IERC20NonStandard(baseToken).balanceOf(address(this)); uint totalSupply_ = presentValueSupply(baseSupplyIndex_, totalSupplyBase); uint totalBorrow_ = presentValueBorrow(baseBorrowIndex_, totalBorrowBase); return signed256(balance) - signed256(totalSupply_) + signed256(totalBorrow_); @@ -760,18 +760,49 @@ contract Comet is CometMainInterface { } /** - * @dev Safe ERC20 transfer in, assumes no fee is charged and amount is transferred + * @dev Safe ERC20 transfer in, if fee is charged the final amount transferred to comet will be returned + * @dev Note: Safely handles non-standard ERC-20 tokens that do not return a value. See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca */ - function doTransferIn(address asset, address from, uint amount) internal { - bool success = ERC20(asset).transferFrom(from, address(this), amount); + function doTransferIn(address asset, address from, uint amount) internal returns (uint) { + uint256 preTransferBalance = IERC20NonStandard(asset).balanceOf(address(this)); + IERC20NonStandard(asset).transferFrom(from, address(this), amount); + bool success; + assembly { + switch returndatasize() + case 0 { // This is a non-standard ERC-20 + success := not(0) // set success to true + } + case 32 { // This is a compliant ERC-20 + returndatacopy(0, 0, 32) + success := mload(0) // Set `success = returndata` of override external call + } + default { // This is an excessively non-compliant ERC-20, revert. + revert(0, 0) + } + } if (!success) revert TransferInFailed(); + return IERC20NonStandard(asset).balanceOf(address(this)) - preTransferBalance; } /** * @dev Safe ERC20 transfer out */ function doTransferOut(address asset, address to, uint amount) internal { - bool success = ERC20(asset).transfer(to, amount); + IERC20NonStandard(asset).transfer(to, amount); + bool success; + assembly { + switch returndatasize() + case 0 { // This is a non-standard ERC-20 + success := not(0) // set success to true + } + case 32 { // This is a compliant ERC-20 + returndatacopy(0, 0, 32) + success := mload(0) // Set `success = returndata` of override external call + } + default { // This is an excessively non-compliant ERC-20, revert. + revert(0, 0) + } + } if (!success) revert TransferOutFailed(); } @@ -827,7 +858,7 @@ contract Comet is CometMainInterface { * @dev Supply an amount of base asset from `from` to dst */ function supplyBase(address from, address dst, uint256 amount) internal { - doTransferIn(baseToken, from, amount); + amount = doTransferIn(baseToken, from, amount); accrueInternal(); @@ -854,7 +885,7 @@ contract Comet is CometMainInterface { * @dev Supply an amount of collateral asset from `from` to dst */ function supplyCollateral(address from, address dst, address asset, uint128 amount) internal { - doTransferIn(asset, from, amount); + amount = safe128(doTransferIn(asset, from, amount)); AssetInfo memory assetInfo = getAssetInfoByAddress(asset); TotalsCollateral memory totals = totalsCollateral[asset]; @@ -1199,7 +1230,7 @@ contract Comet is CometMainInterface { if (reserves >= 0 && uint(reserves) >= targetReserves) revert NotForSale(); // Note: Re-entrancy can skip the reserves check above on a second buyCollateral call. - doTransferIn(baseToken, msg.sender, baseAmount); + baseAmount = doTransferIn(baseToken, msg.sender, baseAmount); uint collateralAmount = quoteCollateral(asset, baseAmount); if (collateralAmount < minAmount) revert TooMuchSlippage(); @@ -1261,7 +1292,7 @@ contract Comet is CometMainInterface { function approveThis(address manager, address asset, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); - ERC20(asset).approve(manager, amount); + IERC20NonStandard(asset).approve(manager, amount); } /** @@ -1322,4 +1353,4 @@ contract Comet is CometMainInterface { default { return(0, returndatasize()) } } } -} +} \ No newline at end of file diff --git a/contracts/IERC20NonStandard.sol b/contracts/IERC20NonStandard.sol index 93dd3e276..f38377eb8 100644 --- a/contracts/IERC20NonStandard.sol +++ b/contracts/IERC20NonStandard.sol @@ -7,6 +7,7 @@ pragma solidity 0.8.15; * See https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca */ interface IERC20NonStandard { + function decimals() external view returns (uint8); function approve(address spender, uint256 amount) external; function transfer(address to, uint256 value) external; function transferFrom(address from, address to, uint256 value) external; From 1844dd8f2ef89b9af21b23482eb39dea9318c44d Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 31 Aug 2023 17:55:57 -0700 Subject: [PATCH 09/31] add some usdt token fees scenario tests --- scenario/SupplyScenario.ts | 157 +++++++++++++++++++++++++++++++++++++ 1 file changed, 157 insertions(+) diff --git a/scenario/SupplyScenario.ts b/scenario/SupplyScenario.ts index a3ec668af..bdde8e8c0 100644 --- a/scenario/SupplyScenario.ts +++ b/scenario/SupplyScenario.ts @@ -2,6 +2,7 @@ import { CometContext, scenario } from './context/CometContext'; import { expect } from 'chai'; import { expectApproximately, expectBase, expectRevertCustom, expectRevertMatches, getExpectedBaseBalance, getInterest, isTriviallySourceable, isValidAssetIndex, MAX_ASSETS, UINT256_MAX } from './utils'; import { ContractReceipt } from 'ethers'; +import { matchesDeployment } from './utils'; // XXX introduce a SupplyCapConstraint to separately test the happy path and revert path instead // of testing them conditionally @@ -136,6 +137,42 @@ scenario( } ); +scenario( + 'Comet#supply > base asset with token fees', + { + tokenBalances: { + albert: { $base: 1000 }, // in units of asset, not wei + }, + filter: async (ctx) => matchesDeployment(ctx, [{ deployment: 'usdt' }]) + }, + async ({ comet, actors }, context, world) => { + // Set fees for USDT for testing + const USDTAdmin = await world.deploymentManager.getSigner(); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + // 10 basis points, and max 10 USDT + await USDT.connect(USDTAdmin).setParams(10, 10); + + const { albert } = actors; + const baseAssetAddress = await comet.baseToken(); + const baseAsset = context.getAssetByAddress(baseAssetAddress); + const scale = (await comet.baseScale()).toBigInt(); + + expect(await baseAsset.balanceOf(albert.address)).to.be.equal(1000n * scale); + + // Albert supplies 100 units of base to Comet + await baseAsset.approve(albert, comet.address); + const txn = await albert.supplyAsset({ asset: baseAsset.address, amount: 1000n * scale }); + + const baseIndexScale = (await comet.baseIndexScale()).toBigInt(); + const baseSupplyIndex = (await comet.totalsBasic()).baseSupplyIndex.toBigInt(); + const baseSupplied = getExpectedBaseBalance(999n * scale, baseIndexScale, baseSupplyIndex); + + expect(await comet.balanceOf(albert.address)).to.be.equal(baseSupplied); + + return txn; // return txn to measure gas + } +); + scenario( 'Comet#supply > repay borrow', { @@ -167,6 +204,48 @@ scenario( } ); +scenario( + 'Comet#supply > repay borrow with token fees', + { + tokenBalances: { + albert: { $base: '==1000' } + }, + cometBalances: { + albert: { $base: -1000 } // in units of asset, not wei + }, + filter: async (ctx) => matchesDeployment(ctx, [{ deployment: 'usdt' }]), + }, + async ({ comet, actors }, context, world) => { + // Set fees for USDT for testing + const USDTAdmin = await world.deploymentManager.getSigner(); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + // 10 basis points, and max 10 USDT + await USDT.connect(USDTAdmin).setParams(10, 10); + + const { albert } = actors; + const baseAssetAddress = await comet.baseToken(); + const baseAsset = context.getAssetByAddress(baseAssetAddress); + const scale = (await comet.baseScale()).toBigInt(); + const utilization = await comet.getUtilization(); + const borrowRate = (await comet.getBorrowRate(utilization)).toBigInt(); + + expectApproximately(await albert.getCometBaseBalance(), -1000n * scale, getInterest(1000n * scale, borrowRate, 1n) + 1n); + + // Albert repays 100 units of base borrow + await baseAsset.approve(albert, comet.address); + const txn = await albert.supplyAsset({ asset: baseAsset.address, amount: 1000n * scale }); + + const alberBal = await comet.balanceOf(albert.address); + const alberBorrow = await comet.borrowBalanceOf(albert.address); + + // XXX all these timings are crazy + // Expect to have -1000000, due to token fee, alber only repay 999 USDT instead of 1000 USDT, thus alber still owe 1 USDT which is 1000000 + expectApproximately(await albert.getCometBaseBalance(), -1000000n, getInterest(1000n * scale, borrowRate, 4n) + 2n); + + return txn; // return txn to measure gas + } +); + scenario( 'Comet#supplyFrom > base asset', { @@ -200,6 +279,46 @@ scenario( } ); +scenario( + 'Comet#supplyFrom > base asset with token fees', + { + tokenBalances: { + albert: { $base: 1000 }, // in units of asset, not wei + }, + filter: async (ctx) => matchesDeployment(ctx, [{ deployment: 'usdt' }]), + }, + async ({ comet, actors }, context, world) => { + // Set fees for USDT for testing + const USDTAdmin = await world.deploymentManager.getSigner(); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + // 10 basis points, and max 10 USDT + await USDT.connect(USDTAdmin).setParams(10, 10); + + const { albert, betty } = actors; + const baseAssetAddress = await comet.baseToken(); + const baseAsset = context.getAssetByAddress(baseAssetAddress); + const scale = (await comet.baseScale()).toBigInt(); + + expect(await baseAsset.balanceOf(albert.address)).to.be.equal(1000n * scale); + expect(await comet.balanceOf(betty.address)).to.be.equal(0n); + + await baseAsset.approve(albert, comet.address); + await albert.allow(betty, true); + + // Betty supplies 100 units of base from Albert + const txn = await betty.supplyAssetFrom({ src: albert.address, dst: betty.address, asset: baseAsset.address, amount: 1000n * scale }); + + const baseIndexScale = (await comet.baseIndexScale()).toBigInt(); + const baseSupplyIndex = (await comet.totalsBasic()).baseSupplyIndex.toBigInt(); + const baseSupplied = getExpectedBaseBalance(999n * scale, baseIndexScale, baseSupplyIndex); + + expect(await baseAsset.balanceOf(albert.address)).to.be.equal(0n); + expect(await comet.balanceOf(betty.address)).to.be.equal(baseSupplied); + + return txn; // return txn to measure gas + } +); + scenario( 'Comet#supplyFrom > repay borrow', { @@ -229,6 +348,44 @@ scenario( } ); +scenario( + 'Comet#supplyFrom > repay borrow with token fees', + { + tokenBalances: { + albert: { $base: 1010 } + }, + cometBalances: { + betty: { $base: '<= -1000' } // in units of asset, not wei + }, + filter: async (ctx) => matchesDeployment(ctx, [{ deployment: 'usdt' }]), + }, + async ({ comet, actors }, context, world) => { + // Set fees for USDT for testing + const USDTAdmin = await world.deploymentManager.getSigner(); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + // 10 basis points, and max 10 USDT + await USDT.connect(USDTAdmin).setParams(10, 10); + + const { albert, betty } = actors; + const baseAssetAddress = await comet.baseToken(); + const baseAsset = context.getAssetByAddress(baseAssetAddress); + const scale = (await comet.baseScale()).toBigInt(); + + await baseAsset.approve(albert, comet.address); + await albert.allow(betty, true); + + // Betty supplies max base from Albert to repay all borrows + const txn = await betty.supplyAssetFrom({ src: albert.address, dst: betty.address, asset: baseAsset.address, amount: UINT256_MAX }); + + expect(await baseAsset.balanceOf(albert.address)).to.be.lessThan(10n * scale); + // This is expected as default behavior is setting amount to be betty borrowBalance, + // But when supplying asset, toke fees takes 0.1% away, which betty will still owe 1 USDT + expectBase(await betty.getCometBaseBalance(), -1000000n); + + return txn; // return txn to measure gas + } +); + scenario( 'Comet#supply reverts if not enough ERC20 approval', { From c25386dd0b000e9ea6b25634f6b9fc8ba5f266d6 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 7 Sep 2023 17:09:27 -0700 Subject: [PATCH 10/31] add liquidation scenario tests --- scenario/LiquidationScenario.ts | 60 +++++++++++++++++++++++++++++++++ scenario/SupplyScenario.ts | 3 -- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/scenario/LiquidationScenario.ts b/scenario/LiquidationScenario.ts index 567c72cf0..166bc3fc8 100644 --- a/scenario/LiquidationScenario.ts +++ b/scenario/LiquidationScenario.ts @@ -1,6 +1,7 @@ import { scenario } from './context/CometContext'; import { event, expect } from '../test/helpers'; import { expectRevertCustom, timeUntilUnderwater } from './utils'; +import { matchesDeployment } from './utils'; scenario( 'Comet#liquidation > isLiquidatable=true for underwater position', @@ -120,6 +121,65 @@ scenario( } ); +scenario( + 'Comet#liquidation > allows liquidation of underwater positions with token fees', + { + tokenBalances: { + $comet: { $base: 1000 }, + }, + cometBalances: { + albert: { + $base: -1000, + $asset0: .001 + }, + betty: { $base: 10 } + }, + filter: async (ctx) => matchesDeployment(ctx, [{ deployment: 'usdt' }]), + }, + async ({ comet, actors }, context, world) => { + // Set fees for USDT for testing + const USDTAdmin = await world.deploymentManager.getSigner(); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), world.base.network); + // 10 basis points, and max 10 USDT + await USDT.connect(USDTAdmin).setParams(10, 10); + + const { albert, betty } = actors; + + await world.increaseTime( + await timeUntilUnderwater({ + comet, + actor: albert, + fudgeFactor: 60n * 10n // 10 minutes past when position is underwater + }) + ); + + const lp0 = await comet.liquidatorPoints(betty.address); + + await betty.absorb({ absorber: betty.address, accounts: [albert.address] }); + + const lp1 = await comet.liquidatorPoints(betty.address); + + // increments absorber's numAbsorbs + expect(lp1.numAbsorbs).to.eq(lp0.numAbsorbs + 1); + // increases absorber's numAbsorbed + expect(lp1.numAbsorbed.toNumber()).to.eq(lp0.numAbsorbed.toNumber() + 1); + // XXX test approxSpend? + + const baseBalance = await albert.getCometBaseBalance(); + expect(Number(baseBalance)).to.be.greaterThanOrEqual(0); + + // clears out all of liquidated user's collateral + const numAssets = await comet.numAssets(); + for (let i = 0; i < numAssets; i++) { + const { asset } = await comet.getAssetInfo(i); + expect(await comet.collateralBalanceOf(albert.address, asset)).to.eq(0); + } + + // clears assetsIn + expect((await comet.userBasic(albert.address)).assetsIn).to.eq(0); + } +); + scenario( 'Comet#liquidation > user can end up with a minted supply', { diff --git a/scenario/SupplyScenario.ts b/scenario/SupplyScenario.ts index bdde8e8c0..e903d93d1 100644 --- a/scenario/SupplyScenario.ts +++ b/scenario/SupplyScenario.ts @@ -235,9 +235,6 @@ scenario( await baseAsset.approve(albert, comet.address); const txn = await albert.supplyAsset({ asset: baseAsset.address, amount: 1000n * scale }); - const alberBal = await comet.balanceOf(albert.address); - const alberBorrow = await comet.borrowBalanceOf(albert.address); - // XXX all these timings are crazy // Expect to have -1000000, due to token fee, alber only repay 999 USDT instead of 1000 USDT, thus alber still owe 1 USDT which is 1000000 expectApproximately(await albert.getCometBaseBalance(), -1000000n, getInterest(1000n * scale, borrowRate, 4n) + 2n); From b4a026e0cb6a7819fc5582629c33df3ac0e0b842 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Fri, 8 Sep 2023 10:44:41 -0700 Subject: [PATCH 11/31] port migration deploy script changes to here --- deployments/goerli/usdt/deploy.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/deployments/goerli/usdt/deploy.ts b/deployments/goerli/usdt/deploy.ts index de00a82e3..cf1538f7e 100644 --- a/deployments/goerli/usdt/deploy.ts +++ b/deployments/goerli/usdt/deploy.ts @@ -6,6 +6,7 @@ const clone = { }; export default async function deploy(deploymentManager: DeploymentManager, deploySpec: DeploySpec): Promise { + const trace = deploymentManager.tracer(); const ethers = deploymentManager.hre.ethers; const signer = await deploymentManager.getSigner(); @@ -19,7 +20,8 @@ export default async function deploy(deploymentManager: DeploymentManager, deplo // Import shared contracts from cUSDCv3 const cometAdmin = await deploymentManager.fromDep('cometAdmin', 'goerli', 'usdc'); - const cometFactory = await deploymentManager.fromDep('cometFactory', 'goerli', 'usdc'); + // Purposely don't use the factory because Comet implementation changed. + // const cometFactory = await deploymentManager.fromDep('cometFactory', 'goerli', 'usdc'); const $configuratorImpl = await deploymentManager.fromDep('configurator:implementation', 'goerli', 'usdc'); const configurator = await deploymentManager.fromDep('configurator', 'goerli', 'usdc'); const rewards = await deploymentManager.fromDep('rewards', 'goerli', 'usdc'); @@ -28,7 +30,22 @@ export default async function deploy(deploymentManager: DeploymentManager, deplo const bulker = await deploymentManager.fromDep('bulker', 'goerli', 'usdc'); const timelock = await deploymentManager.fromDep('timelock', 'goerli', 'usdc'); + + // Send some forked USDT to timelock + await deploymentManager.idempotent( + async () => await USDT.connect(signer).balanceOf(timelock.address) == 0, + async () => { + trace(`Sending USDC to timelock`); + await USDT.connect(signer).transfer( + timelock.address, + exp(50_000_000, 6), + ); + trace(`Sent USDC to timelock completed`); + } + ); + // Deploy all Comet-related contracts const deployed = await deployComet(deploymentManager, deploySpec); + return { ...deployed, bulker, fauceteer, fxRoot }; } \ No newline at end of file From eed10de34d81dd8d1e749dd8fcd3031452bfb7e0 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Fri, 8 Sep 2023 12:02:53 -0700 Subject: [PATCH 12/31] Update scenario/SupplyScenario.ts Co-authored-by: Kevin Cheng --- scenario/SupplyScenario.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scenario/SupplyScenario.ts b/scenario/SupplyScenario.ts index e903d93d1..bed452641 100644 --- a/scenario/SupplyScenario.ts +++ b/scenario/SupplyScenario.ts @@ -302,7 +302,7 @@ scenario( await baseAsset.approve(albert, comet.address); await albert.allow(betty, true); - // Betty supplies 100 units of base from Albert + // Betty supplies 1000 units of base from Albert const txn = await betty.supplyAssetFrom({ src: albert.address, dst: betty.address, asset: baseAsset.address, amount: 1000n * scale }); const baseIndexScale = (await comet.baseIndexScale()).toBigInt(); From 01ed6e03d7654371241b62a3109cfe347033d0c0 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Fri, 8 Sep 2023 12:03:02 -0700 Subject: [PATCH 13/31] Update contracts/Comet.sol Co-authored-by: Kevin Cheng --- contracts/Comet.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index a368fa732..3451b6d56 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -760,7 +760,7 @@ contract Comet is CometMainInterface { } /** - * @dev Safe ERC20 transfer in, if fee is charged the final amount transferred to comet will be returned + * @dev Safe ERC20 transfer in and returns the final amount transferred (taking into account any fees) * @dev Note: Safely handles non-standard ERC-20 tokens that do not return a value. See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca */ function doTransferIn(address asset, address from, uint amount) internal returns (uint) { From c68fc7fb887916936574f225756b18051b7fa92d Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Fri, 8 Sep 2023 12:03:17 -0700 Subject: [PATCH 14/31] Update scenario/SupplyScenario.ts Co-authored-by: Kevin Cheng --- scenario/SupplyScenario.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scenario/SupplyScenario.ts b/scenario/SupplyScenario.ts index bed452641..23a6cb6c2 100644 --- a/scenario/SupplyScenario.ts +++ b/scenario/SupplyScenario.ts @@ -159,7 +159,7 @@ scenario( expect(await baseAsset.balanceOf(albert.address)).to.be.equal(1000n * scale); - // Albert supplies 100 units of base to Comet + // Albert supplies 1000 units of base to Comet await baseAsset.approve(albert, comet.address); const txn = await albert.supplyAsset({ asset: baseAsset.address, amount: 1000n * scale }); From 3b9eeee8f57cb6403eb740e23ed38a9e3b97f27e Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Fri, 8 Sep 2023 12:04:46 -0700 Subject: [PATCH 15/31] Update scenario/SupplyScenario.ts Co-authored-by: Kevin Cheng --- scenario/SupplyScenario.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scenario/SupplyScenario.ts b/scenario/SupplyScenario.ts index 23a6cb6c2..1d0aa916c 100644 --- a/scenario/SupplyScenario.ts +++ b/scenario/SupplyScenario.ts @@ -231,7 +231,7 @@ scenario( expectApproximately(await albert.getCometBaseBalance(), -1000n * scale, getInterest(1000n * scale, borrowRate, 1n) + 1n); - // Albert repays 100 units of base borrow + // Albert repays 1000 units of base borrow await baseAsset.approve(albert, comet.address); const txn = await albert.supplyAsset({ asset: baseAsset.address, amount: 1000n * scale }); From 59d101cf88c3f74630c3b41cc004a90a7ea61f72 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Fri, 8 Sep 2023 13:31:25 -0700 Subject: [PATCH 16/31] address comments --- scenario/SupplyScenario.ts | 52 +++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/scenario/SupplyScenario.ts b/scenario/SupplyScenario.ts index 1d0aa916c..359561ad6 100644 --- a/scenario/SupplyScenario.ts +++ b/scenario/SupplyScenario.ts @@ -3,6 +3,7 @@ import { expect } from 'chai'; import { expectApproximately, expectBase, expectRevertCustom, expectRevertMatches, getExpectedBaseBalance, getInterest, isTriviallySourceable, isValidAssetIndex, MAX_ASSETS, UINT256_MAX } from './utils'; import { ContractReceipt } from 'ethers'; import { matchesDeployment } from './utils'; +import { exp } from '../test/helpers'; // XXX introduce a SupplyCapConstraint to separately test the happy path and revert path instead // of testing them conditionally @@ -148,7 +149,7 @@ scenario( async ({ comet, actors }, context, world) => { // Set fees for USDT for testing const USDTAdmin = await world.deploymentManager.getSigner(); - const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), world.base.network); // 10 basis points, and max 10 USDT await USDT.connect(USDTAdmin).setParams(10, 10); @@ -218,7 +219,7 @@ scenario( async ({ comet, actors }, context, world) => { // Set fees for USDT for testing const USDTAdmin = await world.deploymentManager.getSigner(); - const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), world.base.network); // 10 basis points, and max 10 USDT await USDT.connect(USDTAdmin).setParams(10, 10); @@ -237,7 +238,46 @@ scenario( // XXX all these timings are crazy // Expect to have -1000000, due to token fee, alber only repay 999 USDT instead of 1000 USDT, thus alber still owe 1 USDT which is 1000000 - expectApproximately(await albert.getCometBaseBalance(), -1000000n, getInterest(1000n * scale, borrowRate, 4n) + 2n); + expectApproximately(await albert.getCometBaseBalance(), -1n * exp(1, 6), getInterest(1000n * scale, borrowRate, 4n) + 2n); + + return txn; // return txn to measure gas + } +); + +scenario( + 'Comet#supply > repay all borrow with token fees', + { + tokenBalances: { + albert: { $base: '==1000' } + }, + cometBalances: { + albert: { $base: -999 } // in units of asset, not wei + }, + filter: async (ctx) => matchesDeployment(ctx, [{ deployment: 'usdt' }]), + }, + async ({ comet, actors }, context, world) => { + // Set fees for USDT for testing + const USDTAdmin = await world.deploymentManager.getSigner(); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), world.base.network); + // 10 basis points, and max 10 USDT + await USDT.connect(USDTAdmin).setParams(10, 10); + + const { albert } = actors; + const baseAssetAddress = await comet.baseToken(); + const baseAsset = context.getAssetByAddress(baseAssetAddress); + const scale = (await comet.baseScale()).toBigInt(); + const utilization = await comet.getUtilization(); + const borrowRate = (await comet.getBorrowRate(utilization)).toBigInt(); + + expectApproximately(await albert.getCometBaseBalance(), -999n * scale, getInterest(999n * scale, borrowRate, 1n) + 1n); + + // Albert repays 1000 units of base borrow + await baseAsset.approve(albert, comet.address); + const txn = await albert.supplyAsset({ asset: baseAsset.address, amount: 1000n * scale }); + + // XXX all these timings are crazy + // albert supply 1000 USDT to repay, 1000USDT * (99.9%) = 999 USDT, thus albert should have just enough to repay his debt of 999 USDT. + expectApproximately(await albert.getCometBaseBalance(), 0n, getInterest(1000n * scale, borrowRate, 4n) + 2n); return txn; // return txn to measure gas } @@ -287,7 +327,7 @@ scenario( async ({ comet, actors }, context, world) => { // Set fees for USDT for testing const USDTAdmin = await world.deploymentManager.getSigner(); - const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), world.base.network); // 10 basis points, and max 10 USDT await USDT.connect(USDTAdmin).setParams(10, 10); @@ -359,7 +399,7 @@ scenario( async ({ comet, actors }, context, world) => { // Set fees for USDT for testing const USDTAdmin = await world.deploymentManager.getSigner(); - const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), 'goerli'); + const USDT = await world.deploymentManager.existing('USDT', await comet.baseToken(), world.base.network); // 10 basis points, and max 10 USDT await USDT.connect(USDTAdmin).setParams(10, 10); @@ -377,7 +417,7 @@ scenario( expect(await baseAsset.balanceOf(albert.address)).to.be.lessThan(10n * scale); // This is expected as default behavior is setting amount to be betty borrowBalance, // But when supplying asset, toke fees takes 0.1% away, which betty will still owe 1 USDT - expectBase(await betty.getCometBaseBalance(), -1000000n); + expectBase(await betty.getCometBaseBalance(), -1n * exp(1, 6)); return txn; // return txn to measure gas } From eff6e141d40d095de6787017bf21d61a1efedd66 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Fri, 8 Sep 2023 13:59:26 -0700 Subject: [PATCH 17/31] add docling --- contracts/Comet.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 3451b6d56..063b05a31 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -786,6 +786,7 @@ contract Comet is CometMainInterface { /** * @dev Safe ERC20 transfer out + * @dev Note: Safely handles non-standard ERC-20 tokens that do not return a value. See here: https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca */ function doTransferOut(address asset, address to, uint amount) internal { IERC20NonStandard(asset).transfer(to, amount); From 736aee57e17c3ce12e8ddeddf8fd6badf974405f Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 11 Sep 2023 16:48:29 -0700 Subject: [PATCH 18/31] add unit test for supply/ supply collateral / buy colalteral into unit test --- contracts/test/NonStandardFaucetFeeToken.sol | 102 +++++++++++++++ test/buy-collateral-test.ts | 87 ++++++++++++- test/helpers.ts | 6 +- test/supply-test.ts | 127 ++++++++++++++++++- 4 files changed, 313 insertions(+), 9 deletions(-) create mode 100644 contracts/test/NonStandardFaucetFeeToken.sol diff --git a/contracts/test/NonStandardFaucetFeeToken.sol b/contracts/test/NonStandardFaucetFeeToken.sol new file mode 100644 index 000000000..10382e67f --- /dev/null +++ b/contracts/test/NonStandardFaucetFeeToken.sol @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity 0.8.15; + +/** + * @title Non-standard ERC20 token + * @dev Implementation of the basic standard token. + * See https://github.com/ethereum/EIPs/issues/20 + * @dev With USDT fee token mechanism + * @dev Note: `transfer` and `transferFrom` do not return a boolean + */ +contract NonStandardFeeToken { + string public name; + string public symbol; + uint8 public decimals; + uint256 public totalSupply; + mapping (address => mapping (address => uint256)) public allowance; + mapping(address => uint256) public balanceOf; + event Approval(address indexed owner, address indexed spender, uint256 value); + event Transfer(address indexed from, address indexed to, uint256 value); + event Params(uint feeBasisPoints, uint maxFee); + + // additional variables for use if transaction fees ever became necessary + uint public basisPointsRate = 0; + uint public maximumFee = 0; + + constructor(uint256 _initialAmount, string memory _tokenName, uint8 _decimalUnits, string memory _tokenSymbol) { + totalSupply = _initialAmount; + balanceOf[msg.sender] = _initialAmount; + name = _tokenName; + symbol = _tokenSymbol; + decimals = _decimalUnits; + } + + function transfer(address dst, uint256 amount) external virtual { + require(amount <= balanceOf[msg.sender], "ERC20: transfer amount exceeds balance"); + uint256 fee = amount * basisPointsRate / 10000; + uint256 sendAmount = amount - fee; + if (fee > maximumFee) { + fee = maximumFee; + } + + // For testing purpose, just forward fee to contract itself + if (fee > 0) { + balanceOf[address(this)] = balanceOf[address(this)] + fee; + } + + balanceOf[msg.sender] = balanceOf[msg.sender] - amount; + balanceOf[dst] = balanceOf[dst] + sendAmount; + emit Transfer(msg.sender, dst, sendAmount); + } + + function transferFrom(address src, address dst, uint256 amount) external virtual { + require(amount <= allowance[src][msg.sender], "ERC20: transfer amount exceeds allowance"); + require(amount <= balanceOf[src], "ERC20: transfer amount exceeds balance"); + uint256 fee = amount * basisPointsRate / 10000; + uint256 sendAmount = amount - fee; + if (fee > maximumFee) { + fee = maximumFee; + } + + // For testing purpose, just forward fee to contract itself + if (fee > 0) { + balanceOf[address(this)] = balanceOf[address(this)] + fee; + } + + allowance[src][msg.sender] = allowance[src][msg.sender] - amount; + balanceOf[src] = balanceOf[src] - amount; + balanceOf[dst] = balanceOf[dst] + sendAmount; + emit Transfer(src, dst, sendAmount); + } + + function approve(address _spender, uint256 amount) external returns (bool) { + allowance[msg.sender][_spender] = amount; + emit Approval(msg.sender, _spender, amount); + return true; + } + + // For testing, just don't limit access on setting fees + function setParams(uint256 newBasisPoints, uint256 newMaxFee) public { + basisPointsRate = newBasisPoints; + maximumFee = newMaxFee * (10**decimals); + + emit Params(basisPointsRate, maximumFee); + } +} + +/** + * @title The Compound Faucet Test Token + * @author Compound + * @notice A simple test token that lets anyone get more of it. + */ +contract NonStandardFaucetFeeToken is NonStandardFeeToken { + constructor(uint256 _initialAmount, string memory _tokenName, uint8 _decimalUnits, string memory _tokenSymbol) + NonStandardFeeToken(_initialAmount, _tokenName, _decimalUnits, _tokenSymbol) { + } + + function allocateTo(address _owner, uint256 value) public { + balanceOf[_owner] += value; + totalSupply += value; + emit Transfer(address(this), _owner, value); + } +} diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index ca271af03..9c305c513 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -1,4 +1,4 @@ -import { EvilToken, EvilToken__factory, FaucetToken } from '../build/types'; +import { EvilToken, EvilToken__factory, NonStandardFaucetFeeToken__factory, FaucetToken, NonStandardFaucetFeeToken } from '../build/types'; import { ethers, event, expect, exp, getBlock, makeProtocol, portfolio, ReentryAttack, wait } from './helpers'; describe('buyCollateral', function () { @@ -323,8 +323,89 @@ describe('buyCollateral', function () { await expect(cometAsA.buyCollateral(COMP.address, exp(50, 18), 50e6, alice.address)).to.be.revertedWith("custom error 'Paused()'"); }); - it.skip('buys the correct amount in a fee-like situation', async () => { - // Note: fee-tokens are not currently supported (for efficiency) and should not be added + it('buys the correct amount in a fee-like situation', async () => { + const protocol = await makeProtocol({ + base: 'USDT', + storeFrontPriceFactor: exp(0.5, 18), + targetReserves: 100, + assets: { + USDT: { + initial: 1e6, + decimals: 6, + initialPrice: 1, + factory: (await ethers.getContractFactory('NonStandardFaucetFeeToken')) as NonStandardFaucetFeeToken__factory, + }, + COMP: { + initial: 1e7, + decimals: 18, + initialPrice: 1, + liquidationFactor: exp(0.8, 18), + factory: (await ethers.getContractFactory('NonStandardFaucetFeeToken')) as NonStandardFaucetFeeToken__factory, + }, + } + }); + + const { comet, tokens, users: [alice] } = protocol; + const { USDT, COMP } = tokens; + + // Set both COMP and USDT with 1% fees + // So we can test internal accounting works correctly in both ways: 1. correctly deducting fees from payment during buyCollateral 2. correctly deducting fees from collateral token to buyer + await (COMP as NonStandardFaucetFeeToken).setParams(100, 10000); + await (USDT as NonStandardFaucetFeeToken).setParams(100, 10000); + + const cometAsA = comet.connect(alice); + const baseAsA = USDT.connect(alice); + + // Reserves are at 0 wei + + // Set up token balances and accounting + await USDT.allocateTo(alice.address, 100e6); + await COMP.allocateTo(comet.address, exp(60, 18)); + + const r0 = await comet.getReserves(); + const p0 = await portfolio(protocol, alice.address); + await wait(baseAsA.approve(comet.address, exp(50, 6))); + // Alice buys 50e6 wei USDT worth of COMP + + // Some math writeup for better understanding in each expects number: + // assetPriceDiscount = 1 - (storeFrontPriceFactor * (1 - liquidationFactor)) * assetPrice + // assetPriceDiscount = 1 - (0.5 * (1 - 0.8)) * 1 = 0.9 + // collateralAmount = basePrice * baseAmount / assetPriceDiscount + // collateralAmount = 1 * 50 * (1 - Token Fee) / 0.9 = 1 * 50 * 0.99 / 0.9 = 55 + // actualReceiveCollateral = 55 * (1 - Token Fee) = 55 * 0.99 = 54.45 + const txn = await wait(cometAsA.buyCollateral(COMP.address, exp(50, 18), 50e6, alice.address)); + const p1 = await portfolio(protocol, alice.address); + const r1 = await comet.getReserves(); + + expect(r0).to.be.equal(0n); + expect(r0).to.be.lt(await comet.targetReserves()); + expect(p0.internal).to.be.deep.equal({ USDT: 0n, COMP: 0n }); + expect(p0.external).to.be.deep.equal({ USDT: exp(100, 6), COMP: 0n }); + expect(p1.internal).to.be.deep.equal({ USDT: 0n, COMP: 0n }); + expect(p1.external).to.be.deep.equal({ USDT: exp(50, 6), COMP: 54450000000000000000n }); + expect(r1).to.be.equal(exp(49.5, 6)); // 50 * 0.99 = 49.5 + expect(event(txn, 0)).to.be.deep.equal({ + Transfer: { + from: alice.address, + to: comet.address, + amount: exp(49.5, 6), + } + }); + expect(event(txn, 1)).to.be.deep.equal({ + Transfer: { + from: comet.address, + to: alice.address, + amount: 54450000000000000000n, + } + }); + expect(event(txn, 2)).to.be.deep.equal({ + BuyCollateral: { + buyer: alice.address, + asset: COMP.address, + baseAmount: exp(49.5, 6), + collateralAmount: 55000000000000000000n, + } + }); }); describe('reentrancy', function() { diff --git a/test/helpers.ts b/test/helpers.ts index 31ebf1d81..94c5fb17b 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -30,6 +30,8 @@ import { Configurator__factory, CometHarnessInterface, CometInterface, + NonStandardFaucetFeeToken, + NonStandardFaucetFeeToken__factory, } from '../build/types'; import { BigNumber } from 'ethers'; import { TransactionReceipt, TransactionResponse } from '@ethersproject/abstract-provider'; @@ -58,7 +60,7 @@ export type ProtocolOpts = { supplyCap?: Numeric; initialPrice?: number; priceFeedDecimals?: number; - factory?: FaucetToken__factory | EvilToken__factory | FaucetWETH__factory; + factory?: FaucetToken__factory | EvilToken__factory | FaucetWETH__factory | NonStandardFaucetFeeToken__factory; }; }; name?: string; @@ -96,7 +98,7 @@ export type Protocol = { reward: string; comet: Comet; tokens: { - [symbol: string]: FaucetToken; + [symbol: string]: FaucetToken | NonStandardFaucetFeeToken; }; unsupportedToken: FaucetToken; priceFeeds: { diff --git a/test/supply-test.ts b/test/supply-test.ts index 0987a2f1c..8895bc340 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -1,5 +1,5 @@ -import { ethers, event, expect, exp, makeProtocol, portfolio, ReentryAttack, setTotalsBasic, wait, fastForward } from './helpers'; -import { EvilToken, EvilToken__factory } from '../build/types'; +import { ethers, event, expect, exp, makeProtocol, portfolio, ReentryAttack, setTotalsBasic, wait, fastForward, defaultAssets } from './helpers'; +import { EvilToken, EvilToken__factory, NonStandardFaucetFeeToken__factory, NonStandardFaucetFeeToken } from '../build/types'; describe('supplyTo', function () { it('supplies base from sender if the asset is base', async () => { @@ -405,8 +405,127 @@ describe('supplyTo', function () { await expect(cometAsB.supplyTo(alice.address, COMP.address, ethers.constants.MaxUint256)).to.be.revertedWith("custom error 'InvalidUInt128()'"); }); - it.skip('supplies the correct amount in a fee-like situation', async () => { - // Note: fee-tokens are not currently supported (for efficiency) and should not be added + it('supplies base the correct amount in a fee-like situation', async () => { + const assets = defaultAssets(); + // Add USDT to assets on top of default assets + assets['USDT'] = { + initial: 1e6, + decimals: 6, + factory: (await ethers.getContractFactory('NonStandardFaucetFeeToken')) as NonStandardFaucetFeeToken__factory, + }; + const protocol = await makeProtocol({ base: 'USDT', assets: assets }); + const { comet, tokens, users: [alice, bob] } = protocol; + const { USDT } = tokens; + + // Set fee to 0.1% + await (USDT as NonStandardFaucetFeeToken).setParams(10, 10); + + const _i0 = await USDT.allocateTo(bob.address, 1000e6); + const baseAsB = USDT.connect(bob); + const cometAsB = comet.connect(bob); + + const t0 = await comet.totalsBasic(); + const p0 = await portfolio(protocol, alice.address); + const q0 = await portfolio(protocol, bob.address); + const _a0 = await wait(baseAsB.approve(comet.address, 1000e6)); + const s0 = await wait(cometAsB.supplyTo(alice.address, USDT.address, 1000e6)); + const t1 = await comet.totalsBasic(); + const p1 = await portfolio(protocol, alice.address); + const q1 = await portfolio(protocol, bob.address); + + expect(event(s0, 0)).to.be.deep.equal({ + Transfer: { + from: bob.address, + to: comet.address, + amount: BigInt(999e6), + } + }); + expect(event(s0, 1)).to.be.deep.equal({ + Supply: { + from: bob.address, + dst: alice.address, + amount: BigInt(999e6), + } + }); + expect(event(s0, 2)).to.be.deep.equal({ + Transfer: { + from: ethers.constants.AddressZero, + to: alice.address, + amount: BigInt(999e6), + } + }); + + expect(p0.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: 0n }); + expect(p0.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: 0n }); + expect(q0.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: 0n }); + expect(q0.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: exp(1000, 6) }); + expect(p1.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: exp(999, 6) }); + expect(p1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: 0n }); + expect(q1.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: 0n }); + expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, USDT: 0n }); + expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase.add(999e6)); + expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); + // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(128000); + }); + + it('supplies collateral the correct amount in a fee-like situation', async () => { + const assets = defaultAssets(); + // Add FeeToken Collateral to assets on top of default assets + assets['FeeToken'] = { + initial: 1e8, + decimals: 18, + factory: (await ethers.getContractFactory('NonStandardFaucetFeeToken')) as NonStandardFaucetFeeToken__factory, + }; + + + const protocol = await makeProtocol({ base: 'USDC', assets: assets }); + const { comet, tokens, users: [alice, bob] } = protocol; + const { FeeToken } = tokens; + + // Set fee to 0.1% + await (FeeToken as NonStandardFaucetFeeToken).setParams(10, 10); + + const _i0 = await FeeToken.allocateTo(bob.address, 2000e8); + const baseAsB = FeeToken.connect(bob); + const cometAsB = comet.connect(bob); + + const t0 = await comet.totalsCollateral(FeeToken.address); + const p0 = await portfolio(protocol, alice.address); + const q0 = await portfolio(protocol, bob.address); + const _a0 = await wait(baseAsB.approve(comet.address, 2000e8)); + const s0 = await wait(cometAsB.supplyTo(alice.address, FeeToken.address, 2000e8)); + const t1 = await comet.totalsCollateral(FeeToken.address); + const p1 = await portfolio(protocol, alice.address); + const q1 = await portfolio(protocol, bob.address); + + expect(event(s0, 0)).to.be.deep.equal({ + Transfer: { + from: bob.address, + to: comet.address, + amount: BigInt(1998e8), + } + }); + expect(event(s0, 1)).to.be.deep.equal({ + SupplyCollateral: { + from: bob.address, + dst: alice.address, + asset: FeeToken.address, + amount: BigInt(1998e8), + } + }); + + expect(p0.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); + expect(p0.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); + expect(q0.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); + expect(q0.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: exp(2000, 8) }); + expect(p1.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: exp(1998, 8) }); + expect(p1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); + expect(q1.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); + expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); + expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(1998e8)); + // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(163000); }); it('prevents exceeding the supply cap via re-entrancy', async () => { From e94e6932978ed262bd982de8b48e91fff8161fee Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 11 Sep 2023 16:57:39 -0700 Subject: [PATCH 19/31] addressed comments --- deployments/goerli/usdt/configuration.json | 8 ++++---- deployments/goerli/usdt/deploy.ts | 7 +++---- deployments/goerli/usdt/relations.ts | 9 +-------- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/deployments/goerli/usdt/configuration.json b/deployments/goerli/usdt/configuration.json index da39f77ec..8ea241e61 100644 --- a/deployments/goerli/usdt/configuration.json +++ b/deployments/goerli/usdt/configuration.json @@ -3,7 +3,7 @@ "symbol": "cUSDTv3", "baseToken": "USDT", "baseTokenPriceFeed": "0xAb5c49580294Aff77670F839ea425f5b78ab3Ae7", - "borrowMin": "10e6", + "borrowMin": "1e0", "governor": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", "pauseGuardian": "0x8Fa336EB4bF58Cfc508dEA1B0aeC7336f55B1399", "storeFrontPriceFactor": 0.5, @@ -21,9 +21,9 @@ "rewardToken": "COMP", "tracking": { "indexScale": "1e15", - "baseSupplySpeed": "0.000402083333333e15", - "baseBorrowSpeed": "0.000402083333333e15", - "baseMinForRewards": "10000e6" + "baseSupplySpeed": "0", + "baseBorrowSpeed": "0", + "baseMinForRewards": "500e6" }, "assets": { "COMP": { diff --git a/deployments/goerli/usdt/deploy.ts b/deployments/goerli/usdt/deploy.ts index cf1538f7e..fc82a7c57 100644 --- a/deployments/goerli/usdt/deploy.ts +++ b/deployments/goerli/usdt/deploy.ts @@ -26,7 +26,6 @@ export default async function deploy(deploymentManager: DeploymentManager, deplo const configurator = await deploymentManager.fromDep('configurator', 'goerli', 'usdc'); const rewards = await deploymentManager.fromDep('rewards', 'goerli', 'usdc'); const fauceteer = await deploymentManager.fromDep('fauceteer', 'goerli', 'usdc'); - const fxRoot = await deploymentManager.fromDep('fxRoot', 'goerli', 'usdc'); const bulker = await deploymentManager.fromDep('bulker', 'goerli', 'usdc'); const timelock = await deploymentManager.fromDep('timelock', 'goerli', 'usdc'); @@ -35,17 +34,17 @@ export default async function deploy(deploymentManager: DeploymentManager, deplo await deploymentManager.idempotent( async () => await USDT.connect(signer).balanceOf(timelock.address) == 0, async () => { - trace(`Sending USDC to timelock`); + trace(`Sending USDT to timelock`); await USDT.connect(signer).transfer( timelock.address, exp(50_000_000, 6), ); - trace(`Sent USDC to timelock completed`); + trace(`Sent USDT to timelock completed`); } ); // Deploy all Comet-related contracts const deployed = await deployComet(deploymentManager, deploySpec); - return { ...deployed, bulker, fauceteer, fxRoot }; + return { ...deployed, bulker, fauceteer }; } \ No newline at end of file diff --git a/deployments/goerli/usdt/relations.ts b/deployments/goerli/usdt/relations.ts index b757fd1c0..60b2c24d1 100644 --- a/deployments/goerli/usdt/relations.ts +++ b/deployments/goerli/usdt/relations.ts @@ -1,12 +1,5 @@ import baseRelationConfig from '../../relations'; export default { - ...baseRelationConfig, - fxRoot: { - relations: { - stateSender: { - field: async fxRoot => fxRoot.stateSender() - } - } - }, + ...baseRelationConfig }; From 8ad9799f06798902dedacf564e468c8ad18c0826 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Tue, 12 Sep 2023 12:15:57 -0700 Subject: [PATCH 20/31] Update contracts/test/NonStandardFaucetFeeToken.sol Co-authored-by: Kevin Cheng --- contracts/test/NonStandardFaucetFeeToken.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/test/NonStandardFaucetFeeToken.sol b/contracts/test/NonStandardFaucetFeeToken.sol index 10382e67f..338d56c08 100644 --- a/contracts/test/NonStandardFaucetFeeToken.sol +++ b/contracts/test/NonStandardFaucetFeeToken.sol @@ -13,7 +13,7 @@ contract NonStandardFeeToken { string public symbol; uint8 public decimals; uint256 public totalSupply; - mapping (address => mapping (address => uint256)) public allowance; + mapping(address => mapping (address => uint256)) public allowance; mapping(address => uint256) public balanceOf; event Approval(address indexed owner, address indexed spender, uint256 value); event Transfer(address indexed from, address indexed to, uint256 value); From ba090f0781098b8169f074d1f212e44f5939a6c9 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 12 Sep 2023 12:44:51 -0700 Subject: [PATCH 21/31] address comments --- contracts/test/NonStandardFaucetFeeToken.sol | 6 +++--- test/buy-collateral-test.ts | 6 +++--- test/supply-test.ts | 1 - 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/contracts/test/NonStandardFaucetFeeToken.sol b/contracts/test/NonStandardFaucetFeeToken.sol index 338d56c08..755e62b08 100644 --- a/contracts/test/NonStandardFaucetFeeToken.sol +++ b/contracts/test/NonStandardFaucetFeeToken.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity 0.8.15; +import "../IERC20NonStandard.sol"; /** * @title Non-standard ERC20 token * @dev Implementation of the basic standard token. @@ -8,7 +9,7 @@ pragma solidity 0.8.15; * @dev With USDT fee token mechanism * @dev Note: `transfer` and `transferFrom` do not return a boolean */ -contract NonStandardFeeToken { +contract NonStandardFeeToken is IERC20NonStandard { string public name; string public symbol; uint8 public decimals; @@ -69,10 +70,9 @@ contract NonStandardFeeToken { emit Transfer(src, dst, sendAmount); } - function approve(address _spender, uint256 amount) external returns (bool) { + function approve(address _spender, uint256 amount) external { allowance[msg.sender][_spender] = amount; emit Approval(msg.sender, _spender, amount); - return true; } // For testing, just don't limit access on setting fees diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 9c305c513..6bf2d4008 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -382,7 +382,7 @@ describe('buyCollateral', function () { expect(p0.internal).to.be.deep.equal({ USDT: 0n, COMP: 0n }); expect(p0.external).to.be.deep.equal({ USDT: exp(100, 6), COMP: 0n }); expect(p1.internal).to.be.deep.equal({ USDT: 0n, COMP: 0n }); - expect(p1.external).to.be.deep.equal({ USDT: exp(50, 6), COMP: 54450000000000000000n }); + expect(p1.external).to.be.deep.equal({ USDT: exp(50, 6), COMP: exp(54.45, 18) }); expect(r1).to.be.equal(exp(49.5, 6)); // 50 * 0.99 = 49.5 expect(event(txn, 0)).to.be.deep.equal({ Transfer: { @@ -395,7 +395,7 @@ describe('buyCollateral', function () { Transfer: { from: comet.address, to: alice.address, - amount: 54450000000000000000n, + amount: exp(54.45, 18), } }); expect(event(txn, 2)).to.be.deep.equal({ @@ -403,7 +403,7 @@ describe('buyCollateral', function () { buyer: alice.address, asset: COMP.address, baseAmount: exp(49.5, 6), - collateralAmount: 55000000000000000000n, + collateralAmount: exp(55, 18), } }); }); diff --git a/test/supply-test.ts b/test/supply-test.ts index 8895bc340..679a684af 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -478,7 +478,6 @@ describe('supplyTo', function () { factory: (await ethers.getContractFactory('NonStandardFaucetFeeToken')) as NonStandardFaucetFeeToken__factory, }; - const protocol = await makeProtocol({ base: 'USDC', assets: assets }); const { comet, tokens, users: [alice, bob] } = protocol; const { FeeToken } = tokens; From cd3776f98c9f8966d7905f2255664f6f896f0aa3 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 12 Sep 2023 14:18:29 -0700 Subject: [PATCH 22/31] fixed some tsc error complains --- test/comet-ext-test.ts | 4 ++-- test/helpers.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/comet-ext-test.ts b/test/comet-ext-test.ts index 265130296..d297bf22b 100644 --- a/test/comet-ext-test.ts +++ b/test/comet-ext-test.ts @@ -1,11 +1,11 @@ -import { CometHarnessInterface, FaucetToken } from '../build/types'; +import { CometHarnessInterface, FaucetToken, NonStandardFaucetFeeToken } from '../build/types'; import { expect, exp, makeProtocol, setTotalsBasic } from './helpers'; import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; describe('CometExt', function () { let comet: CometHarnessInterface; let user: SignerWithAddress; - let tokens: { [symbol: string]: FaucetToken }; + let tokens: { [symbol: string]: FaucetToken | NonStandardFaucetFeeToken }; beforeEach(async () => { ({ diff --git a/test/helpers.ts b/test/helpers.ts index 94c5fb17b..e53d6043c 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -116,7 +116,7 @@ export type ConfiguratorAndProtocol = { export type RewardsOpts = { governor?: SignerWithAddress; - configs?: [Comet, FaucetToken, Numeric?][]; + configs?: [Comet, FaucetToken | NonStandardFaucetFeeToken, Numeric?][]; }; export type Rewards = { @@ -505,7 +505,7 @@ export async function makeBulker(opts: BulkerOpts): Promise { bulker }; } -export async function bumpTotalsCollateral(comet: CometHarnessInterface, token: FaucetToken, delta: bigint): Promise { +export async function bumpTotalsCollateral(comet: CometHarnessInterface, token: FaucetToken | NonStandardFaucetFeeToken, delta: bigint): Promise { const t0 = await comet.totalsCollateral(token.address); const t1 = Object.assign({}, t0, { totalSupplyAsset: t0.totalSupplyAsset.toBigInt() + delta }); await token.allocateTo(comet.address, delta); From 12e5e8f6a924c5fc618b4e8484838870646ca446 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 12 Sep 2023 17:35:44 -0700 Subject: [PATCH 23/31] fix re-entry tests, that since evilToken never transfer anything, with new Comet logics that will compare Comet's raw balance diff to credit user in accounting. Comet's always got 0 token, so comet always credit 0, thus it won't hit SupplyCapExceeded() error no more --- test/buy-collateral-test.ts | 4 +++- test/supply-test.ts | 7 +++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 6bf2d4008..438d46dfc 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -541,7 +541,9 @@ describe('buyCollateral', function () { expect(normalTotalsBasic.baseBorrowIndex).to.equal(evilTotalsBasic.baseBorrowIndex); expect(normalTotalsBasic.trackingSupplyIndex).to.equal(evilTotalsBasic.trackingSupplyIndex); expect(normalTotalsBasic.trackingBorrowIndex).to.equal(evilTotalsBasic.trackingBorrowIndex); - expect(normalTotalsBasic.totalSupplyBase).to.equal(evilTotalsBasic.totalSupplyBase); + expect(normalTotalsBasic.totalSupplyBase).to.equal(1000000n); + // Since EvilToken never transfer actual token to Comet, Comet always gives 0 credit + expect(evilTotalsBasic.totalBorrowBase).to.equal(0n); expect(normalTotalsBasic.totalBorrowBase).to.equal(evilTotalsBasic.totalBorrowBase); expect(normalTotalsCollateral.totalSupplyAsset).to.eq(evilTotalsCollateral.totalSupplyAsset); diff --git a/test/supply-test.ts b/test/supply-test.ts index 679a684af..56070a751 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -554,10 +554,9 @@ describe('supplyTo', function () { await EVIL.setAttack(attack); await comet.connect(alice).allow(EVIL.address, true); - - await expect( - comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("custom error 'SupplyCapExceeded()'"); + await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6); + // Since EvilToken never actually transfer token, so Comet always credit it with 0 + expect(await comet.balanceOf(bob.address)).to.be.equal(0n); }); }); From 4b0fc48b3f719966a2fcc3558d8381a559981e3c Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 12 Sep 2023 17:59:02 -0700 Subject: [PATCH 24/31] another test case that need to set it to 0 balance --- test/buy-collateral-test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 438d46dfc..3ec28e6b9 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -557,7 +557,9 @@ describe('buyCollateral', function () { const normalBobPortfolio = await portfolio(normalProtocol, normalBob.address); const evilBobPortfolio = await portfolio(evilProtocol, evilBob.address); - expect(normalBobPortfolio.internal.USDC).to.equal(evilBobPortfolio.internal.EVIL); + expect(normalBobPortfolio.internal.USDC).to.equal(1000000n); + // EvilToken never transfer actual token to Comet, the new Comet credit user based on the actual token reaches to Comet, so it always gives 0 credit + expect(evilBobPortfolio.internal.EVIL).to.equal(0n); }); }); }); From 63f4061dd689ec68a4b3d6391063071cd3b23250 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 14 Sep 2023 17:40:56 -0700 Subject: [PATCH 25/31] EviltToken with more realistic attack, and adjusted test cases, and added one test case to show the known vulnerability (re-entrance when token contract itself is eploited) --- contracts/test/EvilToken.sol | 25 +++++++++++------- contracts/test/FaucetToken.sol | 4 +-- test/buy-collateral-test.ts | 19 +++++++------- test/supply-test.ts | 46 ++++++++++++++++++++++++++++++++-- 4 files changed, 72 insertions(+), 22 deletions(-) diff --git a/contracts/test/EvilToken.sol b/contracts/test/EvilToken.sol index b17b7ae58..ffb2a1ed5 100644 --- a/contracts/test/EvilToken.sol +++ b/contracts/test/EvilToken.sol @@ -52,20 +52,27 @@ contract EvilToken is FaucetToken { attack = attack_; } - function transfer(address, uint256) external override returns (bool) { - return performAttack(); + function transfer(address dst, uint256 amount) public override returns (bool) { + numberOfCalls++; + if (numberOfCalls > attack.maxCalls){ + return super.transfer(dst, amount); + } else { + return performAttack(address(this), dst, amount); + } } - function transferFrom(address, address, uint256) external override returns (bool) { - return performAttack(); + function transferFrom(address src, address dst, uint256 amount) public override returns (bool) { + numberOfCalls++; + if (numberOfCalls > attack.maxCalls) { + return super.transferFrom(src, dst, amount); + } else { + return performAttack(src, dst, amount); + } } - function performAttack() internal returns (bool) { + function performAttack(address src, address dst, uint256 amount) internal returns (bool) { ReentryAttack memory reentryAttack = attack; - numberOfCalls++; - if (numberOfCalls > reentryAttack.maxCalls) { - // do nothing - } else if (reentryAttack.attackType == AttackType.TRANSFER_FROM) { + if (reentryAttack.attackType == AttackType.TRANSFER_FROM) { Comet(payable(msg.sender)).transferFrom( reentryAttack.source, reentryAttack.destination, diff --git a/contracts/test/FaucetToken.sol b/contracts/test/FaucetToken.sol index 5c4a8a4cf..0122e9629 100644 --- a/contracts/test/FaucetToken.sol +++ b/contracts/test/FaucetToken.sol @@ -24,7 +24,7 @@ contract StandardToken { decimals = _decimalUnits; } - function transfer(address dst, uint256 amount) external virtual returns (bool) { + function transfer(address dst, uint256 amount) public virtual returns (bool) { require(amount <= balanceOf[msg.sender], "ERC20: transfer amount exceeds balance"); balanceOf[msg.sender] = balanceOf[msg.sender] - amount; balanceOf[dst] = balanceOf[dst] + amount; @@ -32,7 +32,7 @@ contract StandardToken { return true; } - function transferFrom(address src, address dst, uint256 amount) external virtual returns (bool) { + function transferFrom(address src, address dst, uint256 amount) public virtual returns (bool) { require(amount <= allowance[src][msg.sender], "ERC20: transfer amount exceeds allowance"); require(amount <= balanceOf[src], "ERC20: transfer amount exceeds balance"); allowance[src][msg.sender] = allowance[src][msg.sender] - amount; diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 3ec28e6b9..7b6fa7fd0 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -460,7 +460,7 @@ describe('buyCollateral', function () { source: evilAlice.address, destination: evilBob.address, asset: EVIL.address, - amount: 1e6, + amount: 3000e6, maxCalls: 1 }); await EVIL.setAttack(attack); @@ -490,7 +490,7 @@ describe('buyCollateral', function () { // approve Comet to move funds await normalUSDC.connect(normalAlice).approve(normalComet.address, exp(5000, 6)); await EVIL.connect(evilAlice).approve(EVIL.address, exp(5000, 6)); - + await EVIL.connect(evilAlice).approve(evilComet.address, exp(5000, 6)); // perform the supplies for each protocol in the same block, so that the // same amount of time elapses for each when calculating interest await ethers.provider.send('evm_setAutomine', [false]); @@ -522,10 +522,11 @@ describe('buyCollateral', function () { .connect(evilAlice) .buyCollateral( evilWETH.address, - exp(.5, 18), + exp(0, 18), exp(3000, 6), evilAlice.address ); + await evilComet.accrueAccount(evilAlice.address); // !important; reenable automine @@ -541,9 +542,9 @@ describe('buyCollateral', function () { expect(normalTotalsBasic.baseBorrowIndex).to.equal(evilTotalsBasic.baseBorrowIndex); expect(normalTotalsBasic.trackingSupplyIndex).to.equal(evilTotalsBasic.trackingSupplyIndex); expect(normalTotalsBasic.trackingBorrowIndex).to.equal(evilTotalsBasic.trackingBorrowIndex); - expect(normalTotalsBasic.totalSupplyBase).to.equal(1000000n); - // Since EvilToken never transfer actual token to Comet, Comet always gives 0 credit - expect(evilTotalsBasic.totalBorrowBase).to.equal(0n); + expect(normalTotalsBasic.totalSupplyBase).to.equal(1e6); + // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should have 3000e6 + expect(evilTotalsBasic.totalSupplyBase).to.equal(3000e6); expect(normalTotalsBasic.totalBorrowBase).to.equal(evilTotalsBasic.totalBorrowBase); expect(normalTotalsCollateral.totalSupplyAsset).to.eq(evilTotalsCollateral.totalSupplyAsset); @@ -557,9 +558,9 @@ describe('buyCollateral', function () { const normalBobPortfolio = await portfolio(normalProtocol, normalBob.address); const evilBobPortfolio = await portfolio(evilProtocol, evilBob.address); - expect(normalBobPortfolio.internal.USDC).to.equal(1000000n); - // EvilToken never transfer actual token to Comet, the new Comet credit user based on the actual token reaches to Comet, so it always gives 0 credit - expect(evilBobPortfolio.internal.EVIL).to.equal(0n); + expect(normalBobPortfolio.internal.USDC).to.equal(1e6); + // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should be 3000e6 (under Bob's name) + expect(evilBobPortfolio.internal.EVIL).to.equal(3000e6); }); }); }); diff --git a/test/supply-test.ts b/test/supply-test.ts index 56070a751..fb0e5c976 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -554,9 +554,51 @@ describe('supplyTo', function () { await EVIL.setAttack(attack); await comet.connect(alice).allow(EVIL.address, true); + await wait(EVIL.connect(alice).approve(comet.address, 75e6)); + await EVIL.allocateTo(alice.address, 75e6); + await expect( + comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) + ).to.be.revertedWith("custom error 'SupplyCapExceeded()'"); + }); + + // This test is purposely test the edge case where comet internal accounting will be incorrect when EvilToken re-entrancy attack + // is performed. The attack will cause comet to have inflated supply amount of token. + // + // We decided that this won't be a concern for Comet, as in order to pull off this attack, the governance has to propose and vote to + // add suspicious token in to Comet's market. As long as governance doesn't add suspicious token contract or erc-777 token to market, the + // Comet should not be vulnerable to this type of attack. + it('incorrect accounting via re-entrancy', async () => { + const { comet, tokens, users: [alice, bob] } = await makeProtocol({ + assets: { + USDC: { + decimals: 6 + }, + EVIL: { + decimals: 6, + initialPrice: 2, + factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, + supplyCap: 250e6 + } + } + }); + const { EVIL } = <{ EVIL: EvilToken }>tokens; + + const attack = Object.assign({}, await EVIL.getAttack(), { + attackType: ReentryAttack.SupplyFrom, + source: alice.address, + destination: bob.address, + asset: EVIL.address, + amount: 75e6, + maxCalls: 1 + }); + await EVIL.setAttack(attack); + + await comet.connect(alice).allow(EVIL.address, true); + await wait(EVIL.connect(alice).approve(comet.address, 75e6)); + await EVIL.allocateTo(alice.address, 75e6); await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6); - // Since EvilToken never actually transfer token, so Comet always credit it with 0 - expect(await comet.balanceOf(bob.address)).to.be.equal(0n); + // Re-entrancy attack 2 loops, expect to have 2X accounting balance 75*2 = 150 + expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.equal(150e6); }); }); From 277b2862558c9e2e53921b35c7dc10b2d3e82044 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 14 Sep 2023 17:41:36 -0700 Subject: [PATCH 26/31] add extra line --- contracts/test/NonStandardFaucetFeeToken.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/contracts/test/NonStandardFaucetFeeToken.sol b/contracts/test/NonStandardFaucetFeeToken.sol index 755e62b08..a98eb6691 100644 --- a/contracts/test/NonStandardFaucetFeeToken.sol +++ b/contracts/test/NonStandardFaucetFeeToken.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.15; import "../IERC20NonStandard.sol"; + /** * @title Non-standard ERC20 token * @dev Implementation of the basic standard token. From 49588d6c50eb0d20e02307172b4db0ebe41c67ff Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Fri, 15 Sep 2023 14:17:37 -0700 Subject: [PATCH 27/31] add and leveraged oz's re-entrancy guard --- contracts/Comet.sol | 13 +++--- contracts/ReentrancyGuard.sol | 77 +++++++++++++++++++++++++++++++++++ test/buy-collateral-test.ts | 8 ++-- test/supply-test.ts | 46 ++------------------- test/withdraw-test.ts | 4 +- 5 files changed, 93 insertions(+), 55 deletions(-) create mode 100644 contracts/ReentrancyGuard.sol diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 063b05a31..2579b92c9 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -4,13 +4,14 @@ pragma solidity 0.8.15; import "./CometMainInterface.sol"; import "./IERC20NonStandard.sol"; import "./IPriceFeed.sol"; +import "./ReentrancyGuard.sol"; /** * @title Compound's Comet Contract * @notice An efficient monolithic money market protocol * @author Compound */ -contract Comet is CometMainInterface { +contract Comet is CometMainInterface, ReentrancyGuard { /** General configuration constants **/ /// @notice The admin of the protocol @@ -841,7 +842,7 @@ contract Comet is CometMainInterface { * @dev Supply either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will repay all of `dst`'s accrued base borrow balance */ - function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal { + function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal nonReentrant { if (isSupplyPaused()) revert Paused(); if (!hasPermission(from, operator)) revert Unauthorized(); @@ -952,7 +953,7 @@ contract Comet is CometMainInterface { * @dev Transfer either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will transfer all of `src`'s accrued base balance */ - function transferInternal(address operator, address src, address dst, address asset, uint amount) internal { + function transferInternal(address operator, address src, address dst, address asset, uint amount) internal nonReentrant { if (isTransferPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); if (src == dst) revert NoSelfTransfer(); @@ -1063,7 +1064,7 @@ contract Comet is CometMainInterface { * @dev Withdraw either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will withdraw all of `src`'s accrued base balance */ - function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal { + function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal nonReentrant { if (isWithdrawPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); @@ -1224,7 +1225,7 @@ contract Comet is CometMainInterface { * @param baseAmount The amount of base tokens used to buy the collateral * @param recipient The recipient address */ - function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external { + function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external nonReentrant { if (isBuyPaused()) revert Paused(); int reserves = getReserves(); @@ -1270,7 +1271,7 @@ contract Comet is CometMainInterface { * @param to An address of the receiver of withdrawn reserves * @param amount The amount of reserves to be withdrawn from the protocol */ - function withdrawReserves(address to, uint amount) override external { + function withdrawReserves(address to, uint amount) override external nonReentrant { if (msg.sender != governor) revert Unauthorized(); int reserves = getReserves(); diff --git a/contracts/ReentrancyGuard.sol b/contracts/ReentrancyGuard.sol new file mode 100644 index 000000000..af2b98658 --- /dev/null +++ b/contracts/ReentrancyGuard.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (security/ReentrancyGuard.sol) + +pragma solidity ^0.8.0; + +/** + * @dev Contract module that helps prevent reentrant calls to a function. + * + * Inheriting from `ReentrancyGuard` will make the {nonReentrant} modifier + * available, which can be applied to functions to make sure there are no nested + * (reentrant) calls to them. + * + * Note that because there is a single `nonReentrant` guard, functions marked as + * `nonReentrant` may not call one another. This can be worked around by making + * those functions `private`, and then adding `external` `nonReentrant` entry + * points to them. + * + * TIP: If you would like to learn more about reentrancy and alternative ways + * to protect against it, check out our blog post + * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. + */ +abstract contract ReentrancyGuard { + // Booleans are more expensive than uint256 or any type that takes up a full + // word because each write operation emits an extra SLOAD to first read the + // slot's contents, replace the bits taken up by the boolean, and then write + // back. This is the compiler's defense against contract upgrades and + // pointer aliasing, and it cannot be disabled. + + // The values being non-zero value makes deployment a bit more expensive, + // but in exchange the refund on every call to nonReentrant will be lower in + // amount. Since refunds are capped to a percentage of the total + // transaction's gas, it is best to keep them low in cases like this one, to + // increase the likelihood of the full refund coming into effect. + uint256 private constant _NOT_ENTERED = 1; + uint256 private constant _ENTERED = 2; + + uint256 private _status; + + constructor() { + _status = _NOT_ENTERED; + } + + /** + * @dev Prevents a contract from calling itself, directly or indirectly. + * Calling a `nonReentrant` function from another `nonReentrant` + * function is not supported. It is possible to prevent this from happening + * by making the `nonReentrant` function external, and making it call a + * `private` function that does the actual work. + */ + modifier nonReentrant() { + _nonReentrantBefore(); + _; + _nonReentrantAfter(); + } + + function _nonReentrantBefore() private { + // On the first call to nonReentrant, _status will be _NOT_ENTERED + require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); + + // Any calls to nonReentrant after this point will fail + _status = _ENTERED; + } + + function _nonReentrantAfter() private { + // By storing the original value once again, a refund is triggered (see + // https://eips.ethereum.org/EIPS/eip-2200) + _status = _NOT_ENTERED; + } + + /** + * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates there is a + * `nonReentrant` function in the call stack. + */ + function _reentrancyGuardEntered() internal view returns (bool) { + return _status == _ENTERED; + } +} \ No newline at end of file diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 7b6fa7fd0..c659258ce 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -543,8 +543,8 @@ describe('buyCollateral', function () { expect(normalTotalsBasic.trackingSupplyIndex).to.equal(evilTotalsBasic.trackingSupplyIndex); expect(normalTotalsBasic.trackingBorrowIndex).to.equal(evilTotalsBasic.trackingBorrowIndex); expect(normalTotalsBasic.totalSupplyBase).to.equal(1e6); - // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should have 3000e6 - expect(evilTotalsBasic.totalSupplyBase).to.equal(3000e6); + // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name) + expect(evilTotalsBasic.totalSupplyBase).to.equal(0e6); expect(normalTotalsBasic.totalBorrowBase).to.equal(evilTotalsBasic.totalBorrowBase); expect(normalTotalsCollateral.totalSupplyAsset).to.eq(evilTotalsCollateral.totalSupplyAsset); @@ -559,8 +559,8 @@ describe('buyCollateral', function () { const evilBobPortfolio = await portfolio(evilProtocol, evilBob.address); expect(normalBobPortfolio.internal.USDC).to.equal(1e6); - // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should be 3000e6 (under Bob's name) - expect(evilBobPortfolio.internal.EVIL).to.equal(3000e6); + // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name) + expect(evilBobPortfolio.internal.EVIL).to.equal(0e6); }); }); }); diff --git a/test/supply-test.ts b/test/supply-test.ts index fb0e5c976..1e1283a73 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -466,7 +466,7 @@ describe('supplyTo', function () { expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase.add(999e6)); expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(128000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(131000); }); it('supplies collateral the correct amount in a fee-like situation', async () => { @@ -524,7 +524,7 @@ describe('supplyTo', function () { expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(1998e8)); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(163000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(166000); }); it('prevents exceeding the supply cap via re-entrancy', async () => { @@ -558,47 +558,7 @@ describe('supplyTo', function () { await EVIL.allocateTo(alice.address, 75e6); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("custom error 'SupplyCapExceeded()'"); - }); - - // This test is purposely test the edge case where comet internal accounting will be incorrect when EvilToken re-entrancy attack - // is performed. The attack will cause comet to have inflated supply amount of token. - // - // We decided that this won't be a concern for Comet, as in order to pull off this attack, the governance has to propose and vote to - // add suspicious token in to Comet's market. As long as governance doesn't add suspicious token contract or erc-777 token to market, the - // Comet should not be vulnerable to this type of attack. - it('incorrect accounting via re-entrancy', async () => { - const { comet, tokens, users: [alice, bob] } = await makeProtocol({ - assets: { - USDC: { - decimals: 6 - }, - EVIL: { - decimals: 6, - initialPrice: 2, - factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, - supplyCap: 250e6 - } - } - }); - const { EVIL } = <{ EVIL: EvilToken }>tokens; - - const attack = Object.assign({}, await EVIL.getAttack(), { - attackType: ReentryAttack.SupplyFrom, - source: alice.address, - destination: bob.address, - asset: EVIL.address, - amount: 75e6, - maxCalls: 1 - }); - await EVIL.setAttack(attack); - - await comet.connect(alice).allow(EVIL.address, true); - await wait(EVIL.connect(alice).approve(comet.address, 75e6)); - await EVIL.allocateTo(alice.address, 75e6); - await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6); - // Re-entrancy attack 2 loops, expect to have 2X accounting balance 75*2 = 150 - expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.equal(150e6); + ).to.be.revertedWith("ReentrancyGuard: reentrant call"); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index a28d8735b..49eabc155 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -511,7 +511,7 @@ describe('withdraw', function () { // in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'NotCollateralized()'"); + ).to.be.revertedWith("ReentrancyGuard: reentrant call"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -558,7 +558,7 @@ describe('withdraw', function () { // in callback, EvilToken attempts to withdraw USDC to bob's address await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'NotCollateralized()'"); + ).to.be.revertedWith("ReentrancyGuard: reentrant call"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From d0f016a86d9e6b16bd5cbb970a35019a1d9025d6 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Fri, 15 Sep 2023 19:32:26 -0700 Subject: [PATCH 28/31] reentrancy guard in Comet, isntead of importing external contracts with the risk of storage collision --- contracts/Comet.sol | 27 +++++++++-- contracts/CometCore.sol | 4 ++ contracts/CometMainInterface.sol | 3 +- contracts/CometStorage.sol | 3 ++ contracts/ReentrancyGuard.sol | 77 -------------------------------- test/supply-test.ts | 2 +- test/withdraw-test.ts | 4 +- 7 files changed, 36 insertions(+), 84 deletions(-) delete mode 100644 contracts/ReentrancyGuard.sol diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 2579b92c9..d927521f0 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -4,14 +4,13 @@ pragma solidity 0.8.15; import "./CometMainInterface.sol"; import "./IERC20NonStandard.sol"; import "./IPriceFeed.sol"; -import "./ReentrancyGuard.sol"; /** * @title Compound's Comet Contract * @notice An efficient monolithic money market protocol * @author Compound */ -contract Comet is CometMainInterface, ReentrancyGuard { +contract Comet is CometMainInterface { /** General configuration constants **/ /// @notice The admin of the protocol @@ -213,7 +212,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { lastAccrualTime = getNowInternal(); baseSupplyIndex = BASE_INDEX_SCALE; baseBorrowIndex = BASE_INDEX_SCALE; - + reentrancyGuardStatus = REENTRANCY_GUARD_MUTEX_NOT_ENTERED; // Implicit initialization (not worth increasing contract size) // trackingSupplyIndex = 0; // trackingBorrowIndex = 0; @@ -1341,6 +1340,28 @@ contract Comet is CometMainInterface, ReentrancyGuard { return principal < 0 ? presentValueBorrow(baseBorrowIndex_, unsigned104(-principal)) : 0; } + /** + * @notice Reentrancy guard function and modifier for nonReentrant functions + * @dev Note: reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.3/contracts/security/ReentrancyGuard.sol + */ + modifier nonReentrant() { + _nonReentrantBefore(); + _; + _nonReentrantAfter(); + } + + function _nonReentrantBefore() private { + if (reentrancyGuardStatus == REENTRANCY_GUARD_MUTEX_ENTERED) { + revert ReentrancyGuardReentrantCall(); + } + + reentrancyGuardStatus = REENTRANCY_GUARD_MUTEX_ENTERED; + } + + function _nonReentrantAfter() private { + reentrancyGuardStatus = REENTRANCY_GUARD_MUTEX_NOT_ENTERED; + } + /** * @notice Fallback to calling the extension delegate for everything else */ diff --git a/contracts/CometCore.sol b/contracts/CometCore.sol index 94e17d7f0..927faa952 100644 --- a/contracts/CometCore.sol +++ b/contracts/CometCore.sol @@ -56,6 +56,10 @@ abstract contract CometCore is CometConfiguration, CometStorage, CometMath { /// @dev The scale for factors uint64 internal constant FACTOR_SCALE = 1e18; + /// @dev Reentrancy guard mutex status + uint256 internal constant REENTRANCY_GUARD_MUTEX_NOT_ENTERED = 1; + uint256 internal constant REENTRANCY_GUARD_MUTEX_ENTERED = 2; + /** * @notice Determine if the manager has permission to act on behalf of the owner * @param owner The owner account diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol index 651821908..0dfb63084 100644 --- a/contracts/CometMainInterface.sol +++ b/contracts/CometMainInterface.sol @@ -32,6 +32,7 @@ abstract contract CometMainInterface is CometCore { error TransferInFailed(); error TransferOutFailed(); error Unauthorized(); + error ReentrancyGuardReentrantCall(); event Supply(address indexed from, address indexed dst, uint amount); event Transfer(address indexed from, address indexed to, uint amount); @@ -55,7 +56,7 @@ abstract contract CometMainInterface is CometCore { /// @notice Event emitted when reserves are withdrawn by the governor event WithdrawReserves(address indexed to, uint amount); - + function supply(address asset, uint amount) virtual external; function supplyTo(address dst, address asset, uint amount) virtual external; function supplyFrom(address from, address dst, address asset, uint amount) virtual external; diff --git a/contracts/CometStorage.sol b/contracts/CometStorage.sol index 6a97c7bc5..3b4d37e7f 100644 --- a/contracts/CometStorage.sol +++ b/contracts/CometStorage.sol @@ -73,4 +73,7 @@ contract CometStorage { /// @notice Mapping of magic liquidator points mapping(address => LiquidatorPoints) public liquidatorPoints; + + /// @dev Reentrancy guard mutex status + uint256 internal reentrancyGuardStatus; } diff --git a/contracts/ReentrancyGuard.sol b/contracts/ReentrancyGuard.sol deleted file mode 100644 index af2b98658..000000000 --- a/contracts/ReentrancyGuard.sol +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (security/ReentrancyGuard.sol) - -pragma solidity ^0.8.0; - -/** - * @dev Contract module that helps prevent reentrant calls to a function. - * - * Inheriting from `ReentrancyGuard` will make the {nonReentrant} modifier - * available, which can be applied to functions to make sure there are no nested - * (reentrant) calls to them. - * - * Note that because there is a single `nonReentrant` guard, functions marked as - * `nonReentrant` may not call one another. This can be worked around by making - * those functions `private`, and then adding `external` `nonReentrant` entry - * points to them. - * - * TIP: If you would like to learn more about reentrancy and alternative ways - * to protect against it, check out our blog post - * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. - */ -abstract contract ReentrancyGuard { - // Booleans are more expensive than uint256 or any type that takes up a full - // word because each write operation emits an extra SLOAD to first read the - // slot's contents, replace the bits taken up by the boolean, and then write - // back. This is the compiler's defense against contract upgrades and - // pointer aliasing, and it cannot be disabled. - - // The values being non-zero value makes deployment a bit more expensive, - // but in exchange the refund on every call to nonReentrant will be lower in - // amount. Since refunds are capped to a percentage of the total - // transaction's gas, it is best to keep them low in cases like this one, to - // increase the likelihood of the full refund coming into effect. - uint256 private constant _NOT_ENTERED = 1; - uint256 private constant _ENTERED = 2; - - uint256 private _status; - - constructor() { - _status = _NOT_ENTERED; - } - - /** - * @dev Prevents a contract from calling itself, directly or indirectly. - * Calling a `nonReentrant` function from another `nonReentrant` - * function is not supported. It is possible to prevent this from happening - * by making the `nonReentrant` function external, and making it call a - * `private` function that does the actual work. - */ - modifier nonReentrant() { - _nonReentrantBefore(); - _; - _nonReentrantAfter(); - } - - function _nonReentrantBefore() private { - // On the first call to nonReentrant, _status will be _NOT_ENTERED - require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); - - // Any calls to nonReentrant after this point will fail - _status = _ENTERED; - } - - function _nonReentrantAfter() private { - // By storing the original value once again, a refund is triggered (see - // https://eips.ethereum.org/EIPS/eip-2200) - _status = _NOT_ENTERED; - } - - /** - * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates there is a - * `nonReentrant` function in the call stack. - */ - function _reentrancyGuardEntered() internal view returns (bool) { - return _status == _ENTERED; - } -} \ No newline at end of file diff --git a/test/supply-test.ts b/test/supply-test.ts index 1e1283a73..0bf325275 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -558,7 +558,7 @@ describe('supplyTo', function () { await EVIL.allocateTo(alice.address, 75e6); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'ReentrancyGuardReentrantCall()'"); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index 49eabc155..b2a2a8e77 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -511,7 +511,7 @@ describe('withdraw', function () { // in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'ReentrancyGuardReentrantCall()'"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -558,7 +558,7 @@ describe('withdraw', function () { // in callback, EvilToken attempts to withdraw USDC to bob's address await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'ReentrancyGuardReentrantCall()'"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From 976c81f832f1e729bd9b2bac4cebb093ae5f3d04 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 18 Sep 2023 11:38:33 -0700 Subject: [PATCH 29/31] Revert "reentrancy guard in Comet, isntead of importing external contracts with the risk of storage collision" This reverts commit d0f016a86d9e6b16bd5cbb970a35019a1d9025d6. --- contracts/Comet.sol | 27 ++--------- contracts/CometCore.sol | 4 -- contracts/CometMainInterface.sol | 3 +- contracts/CometStorage.sol | 3 -- contracts/ReentrancyGuard.sol | 77 ++++++++++++++++++++++++++++++++ test/supply-test.ts | 2 +- test/withdraw-test.ts | 4 +- 7 files changed, 84 insertions(+), 36 deletions(-) create mode 100644 contracts/ReentrancyGuard.sol diff --git a/contracts/Comet.sol b/contracts/Comet.sol index d927521f0..2579b92c9 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -4,13 +4,14 @@ pragma solidity 0.8.15; import "./CometMainInterface.sol"; import "./IERC20NonStandard.sol"; import "./IPriceFeed.sol"; +import "./ReentrancyGuard.sol"; /** * @title Compound's Comet Contract * @notice An efficient monolithic money market protocol * @author Compound */ -contract Comet is CometMainInterface { +contract Comet is CometMainInterface, ReentrancyGuard { /** General configuration constants **/ /// @notice The admin of the protocol @@ -212,7 +213,7 @@ contract Comet is CometMainInterface { lastAccrualTime = getNowInternal(); baseSupplyIndex = BASE_INDEX_SCALE; baseBorrowIndex = BASE_INDEX_SCALE; - reentrancyGuardStatus = REENTRANCY_GUARD_MUTEX_NOT_ENTERED; + // Implicit initialization (not worth increasing contract size) // trackingSupplyIndex = 0; // trackingBorrowIndex = 0; @@ -1340,28 +1341,6 @@ contract Comet is CometMainInterface { return principal < 0 ? presentValueBorrow(baseBorrowIndex_, unsigned104(-principal)) : 0; } - /** - * @notice Reentrancy guard function and modifier for nonReentrant functions - * @dev Note: reference: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.3/contracts/security/ReentrancyGuard.sol - */ - modifier nonReentrant() { - _nonReentrantBefore(); - _; - _nonReentrantAfter(); - } - - function _nonReentrantBefore() private { - if (reentrancyGuardStatus == REENTRANCY_GUARD_MUTEX_ENTERED) { - revert ReentrancyGuardReentrantCall(); - } - - reentrancyGuardStatus = REENTRANCY_GUARD_MUTEX_ENTERED; - } - - function _nonReentrantAfter() private { - reentrancyGuardStatus = REENTRANCY_GUARD_MUTEX_NOT_ENTERED; - } - /** * @notice Fallback to calling the extension delegate for everything else */ diff --git a/contracts/CometCore.sol b/contracts/CometCore.sol index 927faa952..94e17d7f0 100644 --- a/contracts/CometCore.sol +++ b/contracts/CometCore.sol @@ -56,10 +56,6 @@ abstract contract CometCore is CometConfiguration, CometStorage, CometMath { /// @dev The scale for factors uint64 internal constant FACTOR_SCALE = 1e18; - /// @dev Reentrancy guard mutex status - uint256 internal constant REENTRANCY_GUARD_MUTEX_NOT_ENTERED = 1; - uint256 internal constant REENTRANCY_GUARD_MUTEX_ENTERED = 2; - /** * @notice Determine if the manager has permission to act on behalf of the owner * @param owner The owner account diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol index 0dfb63084..651821908 100644 --- a/contracts/CometMainInterface.sol +++ b/contracts/CometMainInterface.sol @@ -32,7 +32,6 @@ abstract contract CometMainInterface is CometCore { error TransferInFailed(); error TransferOutFailed(); error Unauthorized(); - error ReentrancyGuardReentrantCall(); event Supply(address indexed from, address indexed dst, uint amount); event Transfer(address indexed from, address indexed to, uint amount); @@ -56,7 +55,7 @@ abstract contract CometMainInterface is CometCore { /// @notice Event emitted when reserves are withdrawn by the governor event WithdrawReserves(address indexed to, uint amount); - + function supply(address asset, uint amount) virtual external; function supplyTo(address dst, address asset, uint amount) virtual external; function supplyFrom(address from, address dst, address asset, uint amount) virtual external; diff --git a/contracts/CometStorage.sol b/contracts/CometStorage.sol index 3b4d37e7f..6a97c7bc5 100644 --- a/contracts/CometStorage.sol +++ b/contracts/CometStorage.sol @@ -73,7 +73,4 @@ contract CometStorage { /// @notice Mapping of magic liquidator points mapping(address => LiquidatorPoints) public liquidatorPoints; - - /// @dev Reentrancy guard mutex status - uint256 internal reentrancyGuardStatus; } diff --git a/contracts/ReentrancyGuard.sol b/contracts/ReentrancyGuard.sol new file mode 100644 index 000000000..af2b98658 --- /dev/null +++ b/contracts/ReentrancyGuard.sol @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.9.0) (security/ReentrancyGuard.sol) + +pragma solidity ^0.8.0; + +/** + * @dev Contract module that helps prevent reentrant calls to a function. + * + * Inheriting from `ReentrancyGuard` will make the {nonReentrant} modifier + * available, which can be applied to functions to make sure there are no nested + * (reentrant) calls to them. + * + * Note that because there is a single `nonReentrant` guard, functions marked as + * `nonReentrant` may not call one another. This can be worked around by making + * those functions `private`, and then adding `external` `nonReentrant` entry + * points to them. + * + * TIP: If you would like to learn more about reentrancy and alternative ways + * to protect against it, check out our blog post + * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. + */ +abstract contract ReentrancyGuard { + // Booleans are more expensive than uint256 or any type that takes up a full + // word because each write operation emits an extra SLOAD to first read the + // slot's contents, replace the bits taken up by the boolean, and then write + // back. This is the compiler's defense against contract upgrades and + // pointer aliasing, and it cannot be disabled. + + // The values being non-zero value makes deployment a bit more expensive, + // but in exchange the refund on every call to nonReentrant will be lower in + // amount. Since refunds are capped to a percentage of the total + // transaction's gas, it is best to keep them low in cases like this one, to + // increase the likelihood of the full refund coming into effect. + uint256 private constant _NOT_ENTERED = 1; + uint256 private constant _ENTERED = 2; + + uint256 private _status; + + constructor() { + _status = _NOT_ENTERED; + } + + /** + * @dev Prevents a contract from calling itself, directly or indirectly. + * Calling a `nonReentrant` function from another `nonReentrant` + * function is not supported. It is possible to prevent this from happening + * by making the `nonReentrant` function external, and making it call a + * `private` function that does the actual work. + */ + modifier nonReentrant() { + _nonReentrantBefore(); + _; + _nonReentrantAfter(); + } + + function _nonReentrantBefore() private { + // On the first call to nonReentrant, _status will be _NOT_ENTERED + require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); + + // Any calls to nonReentrant after this point will fail + _status = _ENTERED; + } + + function _nonReentrantAfter() private { + // By storing the original value once again, a refund is triggered (see + // https://eips.ethereum.org/EIPS/eip-2200) + _status = _NOT_ENTERED; + } + + /** + * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates there is a + * `nonReentrant` function in the call stack. + */ + function _reentrancyGuardEntered() internal view returns (bool) { + return _status == _ENTERED; + } +} \ No newline at end of file diff --git a/test/supply-test.ts b/test/supply-test.ts index 0bf325275..1e1283a73 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -558,7 +558,7 @@ describe('supplyTo', function () { await EVIL.allocateTo(alice.address, 75e6); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("custom error 'ReentrancyGuardReentrantCall()'"); + ).to.be.revertedWith("ReentrancyGuard: reentrant call"); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index b2a2a8e77..49eabc155 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -511,7 +511,7 @@ describe('withdraw', function () { // in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'ReentrancyGuardReentrantCall()'"); + ).to.be.revertedWith("ReentrancyGuard: reentrant call"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -558,7 +558,7 @@ describe('withdraw', function () { // in callback, EvilToken attempts to withdraw USDC to bob's address await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'ReentrancyGuardReentrantCall()'"); + ).to.be.revertedWith("ReentrancyGuard: reentrant call"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From a13050a5495dfe147dfb508f063cfc8bfd641985 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 18 Sep 2023 11:40:13 -0700 Subject: [PATCH 30/31] Revert "add and leveraged oz's re-entrancy guard" This reverts commit 49588d6c50eb0d20e02307172b4db0ebe41c67ff. --- contracts/Comet.sol | 13 +++--- contracts/ReentrancyGuard.sol | 77 ----------------------------------- test/buy-collateral-test.ts | 8 ++-- test/supply-test.ts | 46 +++++++++++++++++++-- test/withdraw-test.ts | 4 +- 5 files changed, 55 insertions(+), 93 deletions(-) delete mode 100644 contracts/ReentrancyGuard.sol diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 2579b92c9..063b05a31 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -4,14 +4,13 @@ pragma solidity 0.8.15; import "./CometMainInterface.sol"; import "./IERC20NonStandard.sol"; import "./IPriceFeed.sol"; -import "./ReentrancyGuard.sol"; /** * @title Compound's Comet Contract * @notice An efficient monolithic money market protocol * @author Compound */ -contract Comet is CometMainInterface, ReentrancyGuard { +contract Comet is CometMainInterface { /** General configuration constants **/ /// @notice The admin of the protocol @@ -842,7 +841,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @dev Supply either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will repay all of `dst`'s accrued base borrow balance */ - function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal nonReentrant { + function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal { if (isSupplyPaused()) revert Paused(); if (!hasPermission(from, operator)) revert Unauthorized(); @@ -953,7 +952,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @dev Transfer either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will transfer all of `src`'s accrued base balance */ - function transferInternal(address operator, address src, address dst, address asset, uint amount) internal nonReentrant { + function transferInternal(address operator, address src, address dst, address asset, uint amount) internal { if (isTransferPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); if (src == dst) revert NoSelfTransfer(); @@ -1064,7 +1063,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @dev Withdraw either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will withdraw all of `src`'s accrued base balance */ - function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal nonReentrant { + function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal { if (isWithdrawPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); @@ -1225,7 +1224,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @param baseAmount The amount of base tokens used to buy the collateral * @param recipient The recipient address */ - function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external nonReentrant { + function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external { if (isBuyPaused()) revert Paused(); int reserves = getReserves(); @@ -1271,7 +1270,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @param to An address of the receiver of withdrawn reserves * @param amount The amount of reserves to be withdrawn from the protocol */ - function withdrawReserves(address to, uint amount) override external nonReentrant { + function withdrawReserves(address to, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); int reserves = getReserves(); diff --git a/contracts/ReentrancyGuard.sol b/contracts/ReentrancyGuard.sol deleted file mode 100644 index af2b98658..000000000 --- a/contracts/ReentrancyGuard.sol +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (security/ReentrancyGuard.sol) - -pragma solidity ^0.8.0; - -/** - * @dev Contract module that helps prevent reentrant calls to a function. - * - * Inheriting from `ReentrancyGuard` will make the {nonReentrant} modifier - * available, which can be applied to functions to make sure there are no nested - * (reentrant) calls to them. - * - * Note that because there is a single `nonReentrant` guard, functions marked as - * `nonReentrant` may not call one another. This can be worked around by making - * those functions `private`, and then adding `external` `nonReentrant` entry - * points to them. - * - * TIP: If you would like to learn more about reentrancy and alternative ways - * to protect against it, check out our blog post - * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. - */ -abstract contract ReentrancyGuard { - // Booleans are more expensive than uint256 or any type that takes up a full - // word because each write operation emits an extra SLOAD to first read the - // slot's contents, replace the bits taken up by the boolean, and then write - // back. This is the compiler's defense against contract upgrades and - // pointer aliasing, and it cannot be disabled. - - // The values being non-zero value makes deployment a bit more expensive, - // but in exchange the refund on every call to nonReentrant will be lower in - // amount. Since refunds are capped to a percentage of the total - // transaction's gas, it is best to keep them low in cases like this one, to - // increase the likelihood of the full refund coming into effect. - uint256 private constant _NOT_ENTERED = 1; - uint256 private constant _ENTERED = 2; - - uint256 private _status; - - constructor() { - _status = _NOT_ENTERED; - } - - /** - * @dev Prevents a contract from calling itself, directly or indirectly. - * Calling a `nonReentrant` function from another `nonReentrant` - * function is not supported. It is possible to prevent this from happening - * by making the `nonReentrant` function external, and making it call a - * `private` function that does the actual work. - */ - modifier nonReentrant() { - _nonReentrantBefore(); - _; - _nonReentrantAfter(); - } - - function _nonReentrantBefore() private { - // On the first call to nonReentrant, _status will be _NOT_ENTERED - require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); - - // Any calls to nonReentrant after this point will fail - _status = _ENTERED; - } - - function _nonReentrantAfter() private { - // By storing the original value once again, a refund is triggered (see - // https://eips.ethereum.org/EIPS/eip-2200) - _status = _NOT_ENTERED; - } - - /** - * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates there is a - * `nonReentrant` function in the call stack. - */ - function _reentrancyGuardEntered() internal view returns (bool) { - return _status == _ENTERED; - } -} \ No newline at end of file diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index c659258ce..7b6fa7fd0 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -543,8 +543,8 @@ describe('buyCollateral', function () { expect(normalTotalsBasic.trackingSupplyIndex).to.equal(evilTotalsBasic.trackingSupplyIndex); expect(normalTotalsBasic.trackingBorrowIndex).to.equal(evilTotalsBasic.trackingBorrowIndex); expect(normalTotalsBasic.totalSupplyBase).to.equal(1e6); - // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name) - expect(evilTotalsBasic.totalSupplyBase).to.equal(0e6); + // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should have 3000e6 + expect(evilTotalsBasic.totalSupplyBase).to.equal(3000e6); expect(normalTotalsBasic.totalBorrowBase).to.equal(evilTotalsBasic.totalBorrowBase); expect(normalTotalsCollateral.totalSupplyAsset).to.eq(evilTotalsCollateral.totalSupplyAsset); @@ -559,8 +559,8 @@ describe('buyCollateral', function () { const evilBobPortfolio = await portfolio(evilProtocol, evilBob.address); expect(normalBobPortfolio.internal.USDC).to.equal(1e6); - // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name) - expect(evilBobPortfolio.internal.EVIL).to.equal(0e6); + // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should be 3000e6 (under Bob's name) + expect(evilBobPortfolio.internal.EVIL).to.equal(3000e6); }); }); }); diff --git a/test/supply-test.ts b/test/supply-test.ts index 1e1283a73..fb0e5c976 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -466,7 +466,7 @@ describe('supplyTo', function () { expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase.add(999e6)); expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(131000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(128000); }); it('supplies collateral the correct amount in a fee-like situation', async () => { @@ -524,7 +524,7 @@ describe('supplyTo', function () { expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(1998e8)); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(166000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(163000); }); it('prevents exceeding the supply cap via re-entrancy', async () => { @@ -558,7 +558,47 @@ describe('supplyTo', function () { await EVIL.allocateTo(alice.address, 75e6); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'SupplyCapExceeded()'"); + }); + + // This test is purposely test the edge case where comet internal accounting will be incorrect when EvilToken re-entrancy attack + // is performed. The attack will cause comet to have inflated supply amount of token. + // + // We decided that this won't be a concern for Comet, as in order to pull off this attack, the governance has to propose and vote to + // add suspicious token in to Comet's market. As long as governance doesn't add suspicious token contract or erc-777 token to market, the + // Comet should not be vulnerable to this type of attack. + it('incorrect accounting via re-entrancy', async () => { + const { comet, tokens, users: [alice, bob] } = await makeProtocol({ + assets: { + USDC: { + decimals: 6 + }, + EVIL: { + decimals: 6, + initialPrice: 2, + factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, + supplyCap: 250e6 + } + } + }); + const { EVIL } = <{ EVIL: EvilToken }>tokens; + + const attack = Object.assign({}, await EVIL.getAttack(), { + attackType: ReentryAttack.SupplyFrom, + source: alice.address, + destination: bob.address, + asset: EVIL.address, + amount: 75e6, + maxCalls: 1 + }); + await EVIL.setAttack(attack); + + await comet.connect(alice).allow(EVIL.address, true); + await wait(EVIL.connect(alice).approve(comet.address, 75e6)); + await EVIL.allocateTo(alice.address, 75e6); + await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6); + // Re-entrancy attack 2 loops, expect to have 2X accounting balance 75*2 = 150 + expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.equal(150e6); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index 49eabc155..a28d8735b 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -511,7 +511,7 @@ describe('withdraw', function () { // in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'NotCollateralized()'"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -558,7 +558,7 @@ describe('withdraw', function () { // in callback, EvilToken attempts to withdraw USDC to bob's address await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'NotCollateralized()'"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From 513f45113ac8b0615f5ea2f08755d5629c23248e Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Tue, 6 Feb 2024 17:18:01 -0800 Subject: [PATCH 31/31] Address OZ's feedback on USDT comet support (#818) Address oz's feedback on USDT comet support --- contracts/Comet.sol | 49 ++++++++++++++++++--- contracts/CometCore.sol | 7 +++ contracts/CometMainInterface.sol | 1 + contracts/IERC20NonStandard.sol | 30 ++++++++++++- contracts/test/EvilToken.sol | 10 ++++- test/buy-collateral-test.ts | 74 +++++++++++++++++++++++++++++--- test/helpers.ts | 3 +- test/supply-test.ts | 54 +++-------------------- test/withdraw-test.ts | 18 ++++---- 9 files changed, 176 insertions(+), 70 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 063b05a31..77bfaa0fe 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -201,6 +201,42 @@ contract Comet is CometMainInterface { (asset14_a, asset14_b) = getPackedAssetInternal(config.assetConfigs, 14); } + /** + * @dev Prevents marked functions from being reentered + */ + modifier nonReentrant() { + nonReentrantBefore(); + _; + nonReentrantAfter(); + } + + /** + * @dev Checks that the reentrancy flag is not set and then sets the flag + */ + function nonReentrantBefore() internal { + bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; + uint256 status; + assembly ("memory-safe") { + status := sload(slot) + } + + if (status == REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked(); + assembly ("memory-safe") { + sstore(slot, REENTRANCY_GUARD_ENTERED) + } + } + + /** + * @dev Unsets the reentrancy flag + */ + function nonReentrantAfter() internal { + bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; + uint256 status; + assembly ("memory-safe") { + sstore(slot, REENTRANCY_GUARD_NOT_ENTERED) + } + } + /** * @notice Initialize storage for the contract * @dev Can be used from constructor or proxy @@ -767,7 +803,7 @@ contract Comet is CometMainInterface { uint256 preTransferBalance = IERC20NonStandard(asset).balanceOf(address(this)); IERC20NonStandard(asset).transferFrom(from, address(this), amount); bool success; - assembly { + assembly ("memory-safe") { switch returndatasize() case 0 { // This is a non-standard ERC-20 success := not(0) // set success to true @@ -791,7 +827,7 @@ contract Comet is CometMainInterface { function doTransferOut(address asset, address to, uint amount) internal { IERC20NonStandard(asset).transfer(to, amount); bool success; - assembly { + assembly ("memory-safe") { switch returndatasize() case 0 { // This is a non-standard ERC-20 success := not(0) // set success to true @@ -841,7 +877,7 @@ contract Comet is CometMainInterface { * @dev Supply either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will repay all of `dst`'s accrued base borrow balance */ - function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal { + function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal nonReentrant { if (isSupplyPaused()) revert Paused(); if (!hasPermission(from, operator)) revert Unauthorized(); @@ -952,7 +988,7 @@ contract Comet is CometMainInterface { * @dev Transfer either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will transfer all of `src`'s accrued base balance */ - function transferInternal(address operator, address src, address dst, address asset, uint amount) internal { + function transferInternal(address operator, address src, address dst, address asset, uint amount) internal nonReentrant { if (isTransferPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); if (src == dst) revert NoSelfTransfer(); @@ -1063,7 +1099,7 @@ contract Comet is CometMainInterface { * @dev Withdraw either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will withdraw all of `src`'s accrued base balance */ - function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal { + function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal nonReentrant { if (isWithdrawPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); @@ -1224,7 +1260,7 @@ contract Comet is CometMainInterface { * @param baseAmount The amount of base tokens used to buy the collateral * @param recipient The recipient address */ - function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external { + function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external nonReentrant { if (isBuyPaused()) revert Paused(); int reserves = getReserves(); @@ -1286,6 +1322,7 @@ contract Comet is CometMainInterface { * @dev Only callable by governor * @dev Note: Setting the `asset` as Comet's address will allow the manager * to withdraw from Comet's Comet balance + * @dev Note: For USDT, if there is non-zero prior allowance, it must be reset to 0 first before setting a new value in proposal * @param asset The asset that the manager will gain approval of * @param manager The account which will be allowed or disallowed * @param amount The amount of an asset to approve diff --git a/contracts/CometCore.sol b/contracts/CometCore.sol index 94e17d7f0..534f2701b 100644 --- a/contracts/CometCore.sol +++ b/contracts/CometCore.sol @@ -56,6 +56,13 @@ abstract contract CometCore is CometConfiguration, CometStorage, CometMath { /// @dev The scale for factors uint64 internal constant FACTOR_SCALE = 1e18; + /// @dev The storage slot for reentrancy guard flags + bytes32 internal constant REENTRANCY_GUARD_FLAG_SLOT = bytes32(keccak256("comet.reentrancy.guard")); + + /// @dev The reentrancy guard statuses + uint256 internal constant REENTRANCY_GUARD_NOT_ENTERED = 0; + uint256 internal constant REENTRANCY_GUARD_ENTERED = 1; + /** * @notice Determine if the manager has permission to act on behalf of the owner * @param owner The owner account diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol index 651821908..5347b22f7 100644 --- a/contracts/CometMainInterface.sol +++ b/contracts/CometMainInterface.sol @@ -25,6 +25,7 @@ abstract contract CometMainInterface is CometCore { error NotForSale(); error NotLiquidatable(); error Paused(); + error ReentrantCallBlocked(); error SupplyCapExceeded(); error TimestampTooLarge(); error TooManyAssets(); diff --git a/contracts/IERC20NonStandard.sol b/contracts/IERC20NonStandard.sol index f38377eb8..8ee78bfce 100644 --- a/contracts/IERC20NonStandard.sol +++ b/contracts/IERC20NonStandard.sol @@ -7,9 +7,37 @@ pragma solidity 0.8.15; * See https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca */ interface IERC20NonStandard { + function name() external view returns (string memory); + function symbol() external view returns (string memory); function decimals() external view returns (uint8); + + /** + * @notice Approve `spender` to transfer up to `amount` from `src` + * @dev This will overwrite the approval amount for `spender` + * and is subject to issues noted [here](https://eips.ethereum.org/EIPS/eip-20#approve) + * @param spender The address of the account which may transfer tokens + * @param amount The number of tokens that are approved (-1 means infinite) + */ function approve(address spender, uint256 amount) external; + + /** + * @notice Transfer `value` tokens from `msg.sender` to `to` + * @param to The address of the destination account + * @param value The number of tokens to transfer + */ function transfer(address to, uint256 value) external; + + /** + * @notice Transfer `value` tokens from `from` to `to` + * @param from The address of the source account + * @param to The address of the destination account + * @param value The number of tokens to transfer + */ function transferFrom(address from, address to, uint256 value) external; + + /** + * @notice Gets the balance of the specified address + * @param account The address from which the balance will be retrieved + */ function balanceOf(address account) external view returns (uint256); -} \ No newline at end of file +} diff --git a/contracts/test/EvilToken.sol b/contracts/test/EvilToken.sol index ffb2a1ed5..5bc7826af 100644 --- a/contracts/test/EvilToken.sol +++ b/contracts/test/EvilToken.sol @@ -13,7 +13,8 @@ contract EvilToken is FaucetToken { enum AttackType { TRANSFER_FROM, WITHDRAW_FROM, - SUPPLY_FROM + SUPPLY_FROM, + BUY_COLLATERAL } struct ReentryAttack { @@ -92,6 +93,13 @@ contract EvilToken is FaucetToken { reentryAttack.asset, reentryAttack.amount ); + } else if (reentryAttack.attackType == AttackType.BUY_COLLATERAL) { + Comet(payable(msg.sender)).buyCollateral( + reentryAttack.asset, + 0, + reentryAttack.amount, + reentryAttack.destination + ); } else { revert("invalid reentry attack"); } diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 7b6fa7fd0..9628ab6a8 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -409,7 +409,7 @@ describe('buyCollateral', function () { }); describe('reentrancy', function() { - it('is not broken by reentrancy supply ', async () => { + it('is not broken by reentrancy supply', async () => { const wethArgs = { initial: 1e4, decimals: 18, @@ -543,8 +543,8 @@ describe('buyCollateral', function () { expect(normalTotalsBasic.trackingSupplyIndex).to.equal(evilTotalsBasic.trackingSupplyIndex); expect(normalTotalsBasic.trackingBorrowIndex).to.equal(evilTotalsBasic.trackingBorrowIndex); expect(normalTotalsBasic.totalSupplyBase).to.equal(1e6); - // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should have 3000e6 - expect(evilTotalsBasic.totalSupplyBase).to.equal(3000e6); + // EvilToken attack should be blocked + expect(evilTotalsBasic.totalSupplyBase).to.equal(0); expect(normalTotalsBasic.totalBorrowBase).to.equal(evilTotalsBasic.totalBorrowBase); expect(normalTotalsCollateral.totalSupplyAsset).to.eq(evilTotalsCollateral.totalSupplyAsset); @@ -559,8 +559,72 @@ describe('buyCollateral', function () { const evilBobPortfolio = await portfolio(evilProtocol, evilBob.address); expect(normalBobPortfolio.internal.USDC).to.equal(1e6); - // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should be 3000e6 (under Bob's name) - expect(evilBobPortfolio.internal.EVIL).to.equal(3000e6); + // EvilToken attack should be blocked, so totalSupplyBase should be 0 + expect(evilBobPortfolio.internal.EVIL).to.equal(0); + }); + + it('reentrant buyCollateral is reverted', async () => { + const wethArgs = { + initial: 1e4, + decimals: 18, + initialPrice: 3000, + }; + const baseTokenArgs = { + decimals: 6, + initial: 1e6, + initialPrice: 1, + }; + + // malicious scenario, EVIL token is base + const evilProtocol = await makeProtocol({ + base: 'EVIL', + assets: { + EVIL: { + ...baseTokenArgs, + factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, + }, + WETH: wethArgs, + }, + targetReserves: 1 + }); + const { + comet: evilComet, + tokens: evilTokens, + users: [evilAlice, evilBob] + } = evilProtocol; + const { WETH: evilWETH, EVIL } = <{ WETH: FaucetToken, EVIL: EvilToken }>evilTokens; + + // add attack to EVIL token + const attack = Object.assign({}, await EVIL.getAttack(), { + attackType: ReentryAttack.BuyCollateral, + source: evilAlice.address, + destination: evilBob.address, + asset: evilWETH.address, + amount: 3000e6, + maxCalls: 1 + }); + await EVIL.setAttack(attack); + + // allocate tokens (evil) + await evilWETH.allocateTo(evilComet.address, exp(100, 18)); + await EVIL.allocateTo(evilAlice.address, exp(5000, 6)); + + // approve Comet to move funds + await EVIL.connect(evilAlice).approve(EVIL.address, exp(5000, 6)); + await EVIL.connect(evilAlice).approve(evilComet.address, exp(5000, 6)); + + // authorize EVIL, since callback will originate from EVIL token address + await evilComet.connect(evilAlice).allow(EVIL.address, true); + + // call buyCollateral; supplyFrom is called in callback + await expect(evilComet + .connect(evilAlice) + .buyCollateral( + evilWETH.address, + exp(0, 18), + exp(3000, 6), + evilAlice.address + )).to.be.revertedWith("custom error 'ReentrantCallBlocked()'"); }); }); }); diff --git a/test/helpers.ts b/test/helpers.ts index e53d6043c..58acf19af 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -44,7 +44,8 @@ export type Numeric = number | bigint; export enum ReentryAttack { TransferFrom = 0, WithdrawFrom = 1, - SupplyFrom = 2 + SupplyFrom = 2, + BuyCollateral = 3, } export type ProtocolOpts = { diff --git a/test/supply-test.ts b/test/supply-test.ts index fb0e5c976..c883fdb8f 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -52,7 +52,7 @@ describe('supplyTo', function () { expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase.add(100e6)); expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(120000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(124000); }); it('supplies max base borrow balance (including accrued) from sender if the asset is base', async () => { @@ -259,7 +259,7 @@ describe('supplyTo', function () { expect(p1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(t1.totalSupplyBase).to.be.equal(109); expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(120000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(124000); }); it('supplies collateral from sender if the asset is collateral', async () => { @@ -305,7 +305,7 @@ describe('supplyTo', function () { expect(q1.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(8e8)); - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(140000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(153000); }); it('calculates base principal correctly', async () => { @@ -466,7 +466,7 @@ describe('supplyTo', function () { expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase.add(999e6)); expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(128000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(151000); }); it('supplies collateral the correct amount in a fee-like situation', async () => { @@ -524,10 +524,10 @@ describe('supplyTo', function () { expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(1998e8)); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(163000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(186000); }); - it('prevents exceeding the supply cap via re-entrancy', async () => { + it('blocks reentrancy from exceeding the supply cap', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { @@ -558,47 +558,7 @@ describe('supplyTo', function () { await EVIL.allocateTo(alice.address, 75e6); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("custom error 'SupplyCapExceeded()'"); - }); - - // This test is purposely test the edge case where comet internal accounting will be incorrect when EvilToken re-entrancy attack - // is performed. The attack will cause comet to have inflated supply amount of token. - // - // We decided that this won't be a concern for Comet, as in order to pull off this attack, the governance has to propose and vote to - // add suspicious token in to Comet's market. As long as governance doesn't add suspicious token contract or erc-777 token to market, the - // Comet should not be vulnerable to this type of attack. - it('incorrect accounting via re-entrancy', async () => { - const { comet, tokens, users: [alice, bob] } = await makeProtocol({ - assets: { - USDC: { - decimals: 6 - }, - EVIL: { - decimals: 6, - initialPrice: 2, - factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, - supplyCap: 250e6 - } - } - }); - const { EVIL } = <{ EVIL: EvilToken }>tokens; - - const attack = Object.assign({}, await EVIL.getAttack(), { - attackType: ReentryAttack.SupplyFrom, - source: alice.address, - destination: bob.address, - asset: EVIL.address, - amount: 75e6, - maxCalls: 1 - }); - await EVIL.setAttack(attack); - - await comet.connect(alice).allow(EVIL.address, true); - await wait(EVIL.connect(alice).approve(comet.address, 75e6)); - await EVIL.allocateTo(alice.address, 75e6); - await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6); - // Re-entrancy attack 2 loops, expect to have 2X accounting balance 75*2 = 150 - expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.equal(150e6); + ).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked'); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index a28d8735b..7fe2c0f71 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -54,7 +54,7 @@ describe('withdrawTo', function () { expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(t1.totalSupplyBase).to.be.equal(0n); expect(t1.totalBorrowBase).to.be.equal(0n); - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(100000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(106000); }); it('does not emit Transfer for 0 burn', async () => { @@ -144,7 +144,7 @@ describe('withdrawTo', function () { expect(b1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(t1.totalSupplyBase).to.be.equal(0n); expect(t1.totalBorrowBase).to.be.equal(exp(50, 6)); - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(110000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(115000); }); it('withdraw max base should withdraw 0 if user has a borrow position', async () => { @@ -190,7 +190,7 @@ describe('withdrawTo', function () { expect(b1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase); expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(110000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(121000); }); // This demonstrates a weird quirk of the present value/principal value rounding down math. @@ -279,7 +279,7 @@ describe('withdrawTo', function () { expect(q1.internal).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n }); expect(t1.totalSupplyAsset).to.be.equal(0n); - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(80000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(85000); }); it('calculates base principal correctly', async () => { @@ -475,7 +475,7 @@ describe('withdraw', function () { }); describe('reentrancy', function () { - it('is not broken by malicious reentrancy transferFrom', async () => { + it('blocks malicious reentrant transferFrom', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { @@ -508,10 +508,10 @@ describe('withdraw', function () { await comet.setCollateralBalance(alice.address, EVIL.address, exp(1, 6)); await comet.connect(alice).allow(EVIL.address, true); - // in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) + // In callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'NotCollateralized()'"); + ).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked'); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -521,7 +521,7 @@ describe('withdraw', function () { expect(await USDC.balanceOf(bob.address)).to.eq(0); }); - it('is not broken by malicious reentrancy withdrawFrom', async () => { + it('blocks malicious reentrant withdrawFrom', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { @@ -558,7 +558,7 @@ describe('withdraw', function () { // in callback, EvilToken attempts to withdraw USDC to bob's address await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'NotCollateralized()'"); + ).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked'); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6);