From 369b36302625b3560a749232b778a9838fb9ef7f Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Sat, 28 Oct 2023 19:52:24 -0700 Subject: [PATCH 01/36] address L-02 --- contracts/Comet.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 063b05a31..08acbcd63 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -767,7 +767,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 +791,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 From f7bf99a9e1d133efbb8527c67f4cf7a5bd9fb829 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Sat, 28 Oct 2023 20:27:30 -0700 Subject: [PATCH 02/36] use ethcall instead of assembly --- contracts/Comet.sol | 36 ++++++------------------------------ 1 file changed, 6 insertions(+), 30 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 08acbcd63..050071dba 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -765,22 +765,10 @@ contract Comet is CometMainInterface { */ 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 ("memory-safe") { - 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) - } + (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(IERC20NonStandard.transferFrom.selector, from, address(this), amount)); + if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { + revert TransferInFailed(); } - if (!success) revert TransferInFailed(); return IERC20NonStandard(asset).balanceOf(address(this)) - preTransferBalance; } @@ -789,22 +777,10 @@ contract Comet is CometMainInterface { * @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); - bool success; - assembly ("memory-safe") { - 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) - } + (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(IERC20NonStandard.transfer.selector, to, amount)); + if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { + revert TransferOutFailed(); } - if (!success) revert TransferOutFailed(); } /** From 3544043feb3db65c72c22195e8b69bf8f1d441ab Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 30 Oct 2023 11:12:11 +0900 Subject: [PATCH 03/36] using low level call, so won't need IERC20Nonstandard as oppose for ERC20. --- contracts/Comet.sol | 21 +++++++++++---------- contracts/IERC20NonStandard.sol | 30 +++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 050071dba..bfa0a579f 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.15; import "./CometMainInterface.sol"; -import "./IERC20NonStandard.sol"; +import "./ERC20.sol"; import "./IPriceFeed.sol"; /** @@ -139,7 +139,7 @@ contract Comet is CometMainInterface { **/ constructor(Configuration memory config) { // Sanity checks - uint8 decimals_ = IERC20NonStandard(config.baseToken).decimals(); + uint8 decimals_ = ERC20(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 (IERC20NonStandard(asset).decimals() != decimals_) revert BadDecimals(); + if (ERC20(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 IERC20NonStandard(asset).balanceOf(address(this)) - totalsCollateral[asset].totalSupplyAsset; + return ERC20(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 = IERC20NonStandard(baseToken).balanceOf(address(this)); + uint balance = ERC20(baseToken).balanceOf(address(this)); uint totalSupply_ = presentValueSupply(baseSupplyIndex_, totalSupplyBase); uint totalBorrow_ = presentValueBorrow(baseBorrowIndex_, totalBorrowBase); return signed256(balance) - signed256(totalSupply_) + signed256(totalBorrow_); @@ -764,12 +764,12 @@ contract Comet is CometMainInterface { * @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) { - uint256 preTransferBalance = IERC20NonStandard(asset).balanceOf(address(this)); - (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(IERC20NonStandard.transferFrom.selector, from, address(this), amount)); + uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); + (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { revert TransferInFailed(); } - return IERC20NonStandard(asset).balanceOf(address(this)) - preTransferBalance; + return ERC20(asset).balanceOf(address(this)) - preTransferBalance; } /** @@ -777,7 +777,7 @@ contract Comet is CometMainInterface { * @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 { - (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(IERC20NonStandard.transfer.selector, to, amount)); + (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { revert TransferOutFailed(); } @@ -1262,6 +1262,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 @@ -1269,7 +1270,7 @@ contract Comet is CometMainInterface { function approveThis(address manager, address asset, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); - IERC20NonStandard(asset).approve(manager, amount); + ERC20(asset).approve(manager, amount); } /** diff --git a/contracts/IERC20NonStandard.sol b/contracts/IERC20NonStandard.sol index f38377eb8..d4da0bba6 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 `amount` tokens from `msg.sender` to `dst` + * @param dst The address of the destination account + * @param amount The number of tokens to transfer + */ function transfer(address to, uint256 value) external; + + /** + * @notice Transfer `amount` tokens from `src` to `dst` + * @param src The address of the source account + * @param dst The address of the destination account + * @param amount The number of tokens to transfer + */ function transferFrom(address from, address to, uint256 value) external; + + /** + * @notice Gets the balance of the specified address + * @param owner The address from which the balance will be retrieved + */ function balanceOf(address account) external view returns (uint256); -} \ No newline at end of file +} From 85be0188fa7a95ba3098842798fa20d79021160c Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Wed, 8 Nov 2023 03:01:48 -0800 Subject: [PATCH 04/36] add reentrancy guard, and updated tests --- contracts/Comet.sol | 30 +++++++++++++++-- contracts/CometCore.sol | 7 ++++ contracts/CometMainInterface.sol | 1 + contracts/IERC20NonStandard.sol | 16 ++++----- test/buy-collateral-test.ts | 10 +++--- test/supply-test.ts | 56 +++++--------------------------- test/withdraw-test.ts | 14 ++++---- 7 files changed, 64 insertions(+), 70 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index bfa0a579f..ecd22e65f 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -201,6 +201,32 @@ contract Comet is CometMainInterface { (asset14_a, asset14_b) = getPackedAssetInternal(config.assetConfigs, 14); } + modifier nonReentrant() { + _nonReentrantBefore(); + _; + _nonReentrantAfter(); + } + + function _nonReentrantBefore() private { + bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; + uint256 status; + assembly { + status := sload(slot) + } + + if(status == COMET_REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked(); + assembly { + sstore(slot, COMET_REENTRANCY_GUARD_ENTERED) + } + } + + function _nonReentrantAfter() private { + bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; + assembly { + sstore(slot, COMET_REENTRANCY_GUARD_NOT_ENTERED) + } + } + /** * @notice Initialize storage for the contract * @dev Can be used from constructor or proxy @@ -763,7 +789,7 @@ contract Comet is CometMainInterface { * @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) { + function doTransferIn(address asset, address from, uint amount) nonReentrant internal returns (uint) { uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { @@ -776,7 +802,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 { + function doTransferOut(address asset, address to, uint amount) nonReentrant internal { (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { revert TransferOutFailed(); diff --git a/contracts/CometCore.sol b/contracts/CometCore.sol index 94e17d7f0..0d955cebe 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 COMET_REENTRANCY_GUARD_FLAG_SLOT = bytes32(keccak256("comet.reentrancy.guard")); + + /// @dev The reentrancy guard status + uint256 internal constant COMET_REENTRANCY_GUARD_NOT_ENTERED = 1; + uint256 internal constant COMET_REENTRANCY_GUARD_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..b0f67b867 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 ReentrantCallBlocked(); event Supply(address indexed from, address indexed dst, uint amount); event Transfer(address indexed from, address indexed to, uint amount); diff --git a/contracts/IERC20NonStandard.sol b/contracts/IERC20NonStandard.sol index d4da0bba6..8ee78bfce 100644 --- a/contracts/IERC20NonStandard.sol +++ b/contracts/IERC20NonStandard.sol @@ -21,23 +21,23 @@ interface IERC20NonStandard { function approve(address spender, uint256 amount) external; /** - * @notice Transfer `amount` tokens from `msg.sender` to `dst` - * @param dst The address of the destination account - * @param amount The number of tokens to transfer + * @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 `amount` tokens from `src` to `dst` - * @param src The address of the source account - * @param dst The address of the destination account - * @param amount The number of tokens to transfer + * @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 owner The address from which the balance will be retrieved + * @param account The address from which the balance will be retrieved */ function balanceOf(address account) external view returns (uint256); } diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 7b6fa7fd0..ca092834f 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('block 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,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 should be blocked, so totalSupplyBase should be 0 + expect(evilBobPortfolio.internal.EVIL).to.equal(0); }); }); }); diff --git a/test/supply-test.ts b/test/supply-test.ts index fb0e5c976..92d986277 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('re-entrancy block', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { @@ -537,7 +537,7 @@ describe('supplyTo', function () { decimals: 6, initialPrice: 2, factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, - supplyCap: 100e6 + supplyCap: 250e6 } } }); @@ -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("custom error 'TransferInFailed()'"); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index a28d8735b..296eb32db 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('block malicious reentrancy transferFrom', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { @@ -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("custom error 'TransferOutFailed()'"); // 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("custom error 'TransferOutFailed()'"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From b2f8d03ff0004cf246392a0e0acb5f41fb12af42 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Wed, 8 Nov 2023 11:49:31 -0800 Subject: [PATCH 05/36] address comments --- contracts/Comet.sol | 2 +- contracts/CometCore.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index ecd22e65f..fa5e89d3b 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -214,7 +214,7 @@ contract Comet is CometMainInterface { status := sload(slot) } - if(status == COMET_REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked(); + if (status == COMET_REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked(); assembly { sstore(slot, COMET_REENTRANCY_GUARD_ENTERED) } diff --git a/contracts/CometCore.sol b/contracts/CometCore.sol index 0d955cebe..c10b00c8b 100644 --- a/contracts/CometCore.sol +++ b/contracts/CometCore.sol @@ -60,7 +60,7 @@ abstract contract CometCore is CometConfiguration, CometStorage, CometMath { bytes32 internal constant COMET_REENTRANCY_GUARD_FLAG_SLOT = bytes32(keccak256("comet.reentrancy.guard")); /// @dev The reentrancy guard status - uint256 internal constant COMET_REENTRANCY_GUARD_NOT_ENTERED = 1; + uint256 internal constant COMET_REENTRANCY_GUARD_NOT_ENTERED = 0; uint256 internal constant COMET_REENTRANCY_GUARD_ENTERED = 2; /** From 2c72dff34a9eabe2b266b6c830a50cf0f48e3c47 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Wed, 8 Nov 2023 12:15:16 -0800 Subject: [PATCH 06/36] inline nonReentrant call --- contracts/Comet.sol | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index fa5e89d3b..8eb4dc324 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -201,12 +201,6 @@ contract Comet is CometMainInterface { (asset14_a, asset14_b) = getPackedAssetInternal(config.assetConfigs, 14); } - modifier nonReentrant() { - _nonReentrantBefore(); - _; - _nonReentrantAfter(); - } - function _nonReentrantBefore() private { bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; uint256 status; @@ -789,12 +783,14 @@ contract Comet is CometMainInterface { * @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) nonReentrant internal returns (uint) { + function doTransferIn(address asset, address from, uint amount) internal returns (uint) { + _nonReentrantBefore(); uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { revert TransferInFailed(); } + _nonReentrantAfter(); return ERC20(asset).balanceOf(address(this)) - preTransferBalance; } @@ -802,11 +798,13 @@ 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) nonReentrant internal { + function doTransferOut(address asset, address to, uint amount) internal { + _nonReentrantBefore(); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { revert TransferOutFailed(); } + _nonReentrantAfter(); } /** From 499dce84b04c6310b859c05b6a706bae91f55b50 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Wed, 8 Nov 2023 12:15:30 -0800 Subject: [PATCH 07/36] update on tests --- test/supply-test.ts | 33 +++++++++++++++++++++++++++++++++ test/withdraw-test.ts | 2 +- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/test/supply-test.ts b/test/supply-test.ts index 92d986277..41f9b45f3 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -662,4 +662,37 @@ describe('supplyFrom', function () { await wait(cometAsB.allow(charlie.address, true)); await expect(cometAsC.supplyFrom(bob.address, alice.address, COMP.address, 7)).to.be.revertedWith("custom error 'Paused()'"); }); + + it('block 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); + expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.revertedWith("custom error 'TransferInFailed()'"); +}); }); \ No newline at end of file diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index 296eb32db..9b80973e3 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -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('block malicious reentrancy withdrawFrom', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { From 3d7fd011398f6788bab4b0fe1c1ee72eeef238bc Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Wed, 8 Nov 2023 12:17:35 -0800 Subject: [PATCH 08/36] intent --- test/supply-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/supply-test.ts b/test/supply-test.ts index 41f9b45f3..f89ac35f8 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -694,5 +694,5 @@ describe('supplyFrom', function () { await EVIL.allocateTo(alice.address, 75e6); await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6); expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.revertedWith("custom error 'TransferInFailed()'"); -}); + }); }); \ No newline at end of file From 79ac88a0279ad50a33a0a526eb691774a6ebc67f Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Wed, 8 Nov 2023 18:45:22 -0800 Subject: [PATCH 09/36] Update test/buy-collateral-test.ts Co-authored-by: Kevin Cheng --- test/buy-collateral-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index ca092834f..75576f13e 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -409,7 +409,7 @@ describe('buyCollateral', function () { }); describe('reentrancy', function() { - it('block reentrancy supply ', async () => { + it('is blocked during reentrant supply', async () => { const wethArgs = { initial: 1e4, decimals: 18, From 5ef9cfdabaf21a0db96188da8c0ecf56f5f6e813 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Wed, 8 Nov 2023 18:45:29 -0800 Subject: [PATCH 10/36] Update test/supply-test.ts Co-authored-by: Kevin Cheng --- test/supply-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/supply-test.ts b/test/supply-test.ts index f89ac35f8..6149eca28 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -527,7 +527,7 @@ describe('supplyTo', function () { expect(Number(s0.receipt.gasUsed)).to.be.lessThan(186000); }); - it('re-entrancy block', async () => { + it('blocks reentrancy from exceeding the supply cap', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { From 6a936d1754ebc9194b3f062e8efa96b1d346e822 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Wed, 8 Nov 2023 18:45:35 -0800 Subject: [PATCH 11/36] Update test/withdraw-test.ts Co-authored-by: Kevin Cheng --- test/withdraw-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index 9b80973e3..ad92ea372 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -475,7 +475,7 @@ describe('withdraw', function () { }); describe('reentrancy', function () { - it('block malicious reentrancy transferFrom', async () => { + it('blocks malicious reentrant transferFrom', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { From b3dbaa3a21f6c7b1b769718f0cc41159ba3fffe3 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Wed, 8 Nov 2023 18:45:41 -0800 Subject: [PATCH 12/36] Update test/withdraw-test.ts Co-authored-by: Kevin Cheng --- test/withdraw-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index ad92ea372..caeff1e42 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -521,7 +521,7 @@ describe('withdraw', function () { expect(await USDC.balanceOf(bob.address)).to.eq(0); }); - it('block malicious reentrancy withdrawFrom', async () => { + it('blocks malicious reentrant withdrawFrom', async () => { const { comet, tokens, users: [alice, bob] } = await makeProtocol({ assets: { USDC: { From ad99f2e750fafbc70994bbd07aeedc89f8b4480f Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 9 Nov 2023 00:11:05 -0800 Subject: [PATCH 13/36] address comments --- contracts/Comet.sol | 15 ++++----------- contracts/CometCore.sol | 2 +- test/supply-test.ts | 5 ++--- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 8eb4dc324..2aa12bc2f 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -201,7 +201,7 @@ contract Comet is CometMainInterface { (asset14_a, asset14_b) = getPackedAssetInternal(config.assetConfigs, 14); } - function _nonReentrantBefore() private { + modifier nonReentrant() { bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; uint256 status; assembly { @@ -212,10 +212,7 @@ contract Comet is CometMainInterface { assembly { sstore(slot, COMET_REENTRANCY_GUARD_ENTERED) } - } - - function _nonReentrantAfter() private { - bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; + _; assembly { sstore(slot, COMET_REENTRANCY_GUARD_NOT_ENTERED) } @@ -783,14 +780,12 @@ contract Comet is CometMainInterface { * @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) { - _nonReentrantBefore(); + function doTransferIn(address asset, address from, uint amount) nonReentrant internal returns (uint) { uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { revert TransferInFailed(); } - _nonReentrantAfter(); return ERC20(asset).balanceOf(address(this)) - preTransferBalance; } @@ -798,13 +793,11 @@ 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 { - _nonReentrantBefore(); + function doTransferOut(address asset, address to, uint amount) nonReentrant internal { (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { revert TransferOutFailed(); } - _nonReentrantAfter(); } /** diff --git a/contracts/CometCore.sol b/contracts/CometCore.sol index c10b00c8b..3ff2ada40 100644 --- a/contracts/CometCore.sol +++ b/contracts/CometCore.sol @@ -61,7 +61,7 @@ abstract contract CometCore is CometConfiguration, CometStorage, CometMath { /// @dev The reentrancy guard status uint256 internal constant COMET_REENTRANCY_GUARD_NOT_ENTERED = 0; - uint256 internal constant COMET_REENTRANCY_GUARD_ENTERED = 2; + uint256 internal constant COMET_REENTRANCY_GUARD_ENTERED = 1; /** * @notice Determine if the manager has permission to act on behalf of the owner diff --git a/test/supply-test.ts b/test/supply-test.ts index 6149eca28..80e55fa25 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -537,7 +537,7 @@ describe('supplyTo', function () { decimals: 6, initialPrice: 2, factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, - supplyCap: 250e6 + supplyCap: 100e6 } } }); @@ -692,7 +692,6 @@ describe('supplyFrom', function () { 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); - expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.revertedWith("custom error 'TransferInFailed()'"); + await expect(comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6)).to.be.revertedWith("custom error 'TransferInFailed()'"); }); }); \ No newline at end of file From f2707f2f614efb80a98eea54b87251c4458abd7b Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 9 Nov 2023 00:49:18 -0800 Subject: [PATCH 14/36] add low level support to approveThis as well --- contracts/Comet.sol | 9 ++++++--- contracts/CometMainInterface.sol | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 2aa12bc2f..4173101e7 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -783,7 +783,7 @@ contract Comet is CometMainInterface { function doTransferIn(address asset, address from, uint amount) nonReentrant internal returns (uint) { uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); - if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { + if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { revert TransferInFailed(); } return ERC20(asset).balanceOf(address(this)) - preTransferBalance; @@ -795,7 +795,7 @@ contract Comet is CometMainInterface { */ function doTransferOut(address asset, address to, uint amount) nonReentrant internal { (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); - if (!success || !(returndata.length == 0 || abi.decode(returndata, (bool)))) { + if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { revert TransferOutFailed(); } } @@ -1287,7 +1287,10 @@ 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); + (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.approve.selector, manager, amount)); + if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { + revert ApproveFailed(); + } } /** diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol index b0f67b867..610dfd826 100644 --- a/contracts/CometMainInterface.sol +++ b/contracts/CometMainInterface.sol @@ -11,6 +11,7 @@ import "./CometCore.sol"; abstract contract CometMainInterface is CometCore { error Absurd(); error AlreadyInitialized(); + error ApproveFailed(); error BadAsset(); error BadDecimals(); error BadDiscount(); @@ -25,6 +26,7 @@ abstract contract CometMainInterface is CometCore { error NotForSale(); error NotLiquidatable(); error Paused(); + error ReentrantCallBlocked(); error SupplyCapExceeded(); error TimestampTooLarge(); error TooManyAssets(); @@ -32,7 +34,6 @@ abstract contract CometMainInterface is CometCore { error TransferInFailed(); error TransferOutFailed(); error Unauthorized(); - error ReentrantCallBlocked(); event Supply(address indexed from, address indexed dst, uint amount); event Transfer(address indexed from, address indexed to, uint amount); From 9ff76603d3761938e2cc0bc975bff0b7ac36e9bc Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 9 Nov 2023 01:00:24 -0800 Subject: [PATCH 15/36] add bytes returndata to TransferOut/In() error, remove duplicated tests --- contracts/Comet.sol | 4 ++-- contracts/CometMainInterface.sol | 4 ++-- test/supply-test.ts | 32 -------------------------------- 3 files changed, 4 insertions(+), 36 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 4173101e7..c8dfc859a 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -784,7 +784,7 @@ contract Comet is CometMainInterface { uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { - revert TransferInFailed(); + revert TransferInFailed(returndata); } return ERC20(asset).balanceOf(address(this)) - preTransferBalance; } @@ -796,7 +796,7 @@ contract Comet is CometMainInterface { function doTransferOut(address asset, address to, uint amount) nonReentrant internal { (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { - revert TransferOutFailed(); + revert TransferOutFailed(returndata); } } diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol index 610dfd826..8fe665493 100644 --- a/contracts/CometMainInterface.sol +++ b/contracts/CometMainInterface.sol @@ -31,8 +31,8 @@ abstract contract CometMainInterface is CometCore { error TimestampTooLarge(); error TooManyAssets(); error TooMuchSlippage(); - error TransferInFailed(); - error TransferOutFailed(); + error TransferInFailed(bytes error); + error TransferOutFailed(bytes error); error Unauthorized(); event Supply(address indexed from, address indexed dst, uint amount); diff --git a/test/supply-test.ts b/test/supply-test.ts index 80e55fa25..751e80c6a 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -662,36 +662,4 @@ describe('supplyFrom', function () { await wait(cometAsB.allow(charlie.address, true)); await expect(cometAsC.supplyFrom(bob.address, alice.address, COMP.address, 7)).to.be.revertedWith("custom error 'Paused()'"); }); - - it('block 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 expect(comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6)).to.be.revertedWith("custom error 'TransferInFailed()'"); - }); }); \ No newline at end of file From 41ee8535ad24a256ef34bed9b2b33e9b109b5acf Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 9 Nov 2023 02:25:14 -0800 Subject: [PATCH 16/36] address comments, but now getting contract oversize :( both big contracts went over 24.576kb --- contracts/Comet.sol | 12 ++++++------ test/supply-test.ts | 4 +++- test/withdraw-test.ts | 8 +++++--- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index c8dfc859a..b49db68e6 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -780,7 +780,7 @@ contract Comet is CometMainInterface { * @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) nonReentrant internal returns (uint) { + function doTransferIn(address asset, address from, uint amount) internal returns (uint) { uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { @@ -793,7 +793,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) nonReentrant internal { + function doTransferOut(address asset, address to, uint amount) internal { (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { revert TransferOutFailed(returndata); @@ -834,7 +834,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(); @@ -945,7 +945,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(); @@ -1056,7 +1056,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(); @@ -1217,7 +1217,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(); diff --git a/test/supply-test.ts b/test/supply-test.ts index 751e80c6a..edbca98ef 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -1,5 +1,6 @@ import { ethers, event, expect, exp, makeProtocol, portfolio, ReentryAttack, setTotalsBasic, wait, fastForward, defaultAssets } from './helpers'; import { EvilToken, EvilToken__factory, NonStandardFaucetFeeToken__factory, NonStandardFaucetFeeToken } from '../build/types'; +import { AbiCoder } from 'ethers/lib/utils'; describe('supplyTo', function () { it('supplies base from sender if the asset is base', async () => { @@ -556,9 +557,10 @@ describe('supplyTo', function () { await comet.connect(alice).allow(EVIL.address, true); await wait(EVIL.connect(alice).approve(comet.address, 75e6)); await EVIL.allocateTo(alice.address, 75e6); + const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0,10); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("custom error 'TransferInFailed()'"); + ).to.be.revertedWithCustomError(comet, 'TransferInFailed').withArgs(ReentrantCallBlockedErrorSelector); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index caeff1e42..5261e1fb8 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -508,10 +508,11 @@ 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) + const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0, 10); await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'TransferOutFailed()'"); + ).to.be.revertedWithCustomError(comet, 'TransferOutFailed').withArgs(ReentrantCallBlockedErrorSelector); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -556,9 +557,10 @@ describe('withdraw', function () { await comet.connect(alice).allow(EVIL.address, true); // in callback, EvilToken attempts to withdraw USDC to bob's address + const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0, 10); await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("custom error 'TransferOutFailed()'"); + ).to.be.revertedWithCustomError(comet, 'TransferOutFailed').withArgs(ReentrantCallBlockedErrorSelector); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From 8856e82142a7b74925e804b087bbabaa063ea971 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 9 Nov 2023 16:40:19 -0800 Subject: [PATCH 17/36] trim down a bit on contract size --- contracts/Comet.sol | 21 ++++++++++++--------- contracts/CometMainInterface.sol | 4 +--- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index b49db68e6..23a536942 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -783,9 +783,7 @@ contract Comet is CometMainInterface { function doTransferIn(address asset, address from, uint amount) internal returns (uint) { uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); - if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { - revert TransferInFailed(returndata); - } + optionalReturnBoolCall(success, returndata); return ERC20(asset).balanceOf(address(this)) - preTransferBalance; } @@ -795,8 +793,16 @@ contract Comet is CometMainInterface { */ function doTransferOut(address asset, address to, uint amount) internal { (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); - if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { - revert TransferOutFailed(returndata); + optionalReturnBoolCall(success, returndata); + } + + function optionalReturnBoolCall(bool success, bytes memory returndata) private pure { + if (!success) { + revert TransferFailed(returndata); + } else if (returndata.length != 0) { + if (!abi.decode(returndata, (bool))) { + revert TransferFailed(returndata); + } } } @@ -1287,10 +1293,7 @@ contract Comet is CometMainInterface { function approveThis(address manager, address asset, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); - (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.approve.selector, manager, amount)); - if (!success || (returndata.length != 0 && !abi.decode(returndata, (bool)))) { - revert ApproveFailed(); - } + asset.call(abi.encodeWithSelector(ERC20.approve.selector, manager, amount)); } /** diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol index 8fe665493..1635157b5 100644 --- a/contracts/CometMainInterface.sol +++ b/contracts/CometMainInterface.sol @@ -11,7 +11,6 @@ import "./CometCore.sol"; abstract contract CometMainInterface is CometCore { error Absurd(); error AlreadyInitialized(); - error ApproveFailed(); error BadAsset(); error BadDecimals(); error BadDiscount(); @@ -31,8 +30,7 @@ abstract contract CometMainInterface is CometCore { error TimestampTooLarge(); error TooManyAssets(); error TooMuchSlippage(); - error TransferInFailed(bytes error); - error TransferOutFailed(bytes error); + error TransferFailed(bytes error); error Unauthorized(); event Supply(address indexed from, address indexed dst, uint amount); From e6d6a869bc055ff643dce04363aa625aa0c6dfe0 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 9 Nov 2023 17:00:12 -0800 Subject: [PATCH 18/36] fix tests --- test/supply-test.ts | 2 +- test/withdraw-test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/supply-test.ts b/test/supply-test.ts index edbca98ef..90aafd4c0 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -560,7 +560,7 @@ describe('supplyTo', function () { const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0,10); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWithCustomError(comet, 'TransferInFailed').withArgs(ReentrantCallBlockedErrorSelector); + ).to.be.revertedWithCustomError(comet, 'TransferFailed').withArgs(ReentrantCallBlockedErrorSelector); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index 5261e1fb8..cd953b89c 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -512,7 +512,7 @@ describe('withdraw', function () { const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0, 10); await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWithCustomError(comet, 'TransferOutFailed').withArgs(ReentrantCallBlockedErrorSelector); + ).to.be.revertedWithCustomError(comet, 'TransferFailed').withArgs(ReentrantCallBlockedErrorSelector); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -560,7 +560,7 @@ describe('withdraw', function () { const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0, 10); await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWithCustomError(comet, 'TransferOutFailed').withArgs(ReentrantCallBlockedErrorSelector); + ).to.be.revertedWithCustomError(comet, 'TransferFailed').withArgs(ReentrantCallBlockedErrorSelector); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From 9f770b44a28f326429e3a97129df5d07b3713593 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Thu, 9 Nov 2023 17:15:05 -0800 Subject: [PATCH 19/36] fix lint --- test/supply-test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/supply-test.ts b/test/supply-test.ts index 90aafd4c0..65d9da800 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -1,6 +1,5 @@ import { ethers, event, expect, exp, makeProtocol, portfolio, ReentryAttack, setTotalsBasic, wait, fastForward, defaultAssets } from './helpers'; import { EvilToken, EvilToken__factory, NonStandardFaucetFeeToken__factory, NonStandardFaucetFeeToken } from '../build/types'; -import { AbiCoder } from 'ethers/lib/utils'; describe('supplyTo', function () { it('supplies base from sender if the asset is base', async () => { From 46896c9418f43062c9492a0529c9ad27b2594dc6 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 13 Nov 2023 13:38:56 -0800 Subject: [PATCH 20/36] use assembly for transfer action, as it save more spaces, and it's already in-use in other contracts --- contracts/Comet.sol | 60 +++++++++++++++++++++----------- contracts/CometMainInterface.sol | 3 +- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 23a536942..ed5ec2760 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(); @@ -258,7 +258,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(); @@ -499,7 +499,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; } /** @@ -507,7 +507,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_); @@ -781,10 +781,24 @@ contract Comet is CometMainInterface { * @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) { - uint256 preTransferBalance = ERC20(asset).balanceOf(address(this)); - (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transferFrom.selector, from, address(this), amount)); - optionalReturnBoolCall(success, returndata); - return ERC20(asset).balanceOf(address(this)) - preTransferBalance; + 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; } /** @@ -792,18 +806,22 @@ contract Comet is CometMainInterface { * @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 { - (bool success, bytes memory returndata) = asset.call(abi.encodeWithSelector(ERC20.transfer.selector, to, amount)); - optionalReturnBoolCall(success, returndata); - } - - function optionalReturnBoolCall(bool success, bytes memory returndata) private pure { - if (!success) { - revert TransferFailed(returndata); - } else if (returndata.length != 0) { - if (!abi.decode(returndata, (bool))) { - revert TransferFailed(returndata); - } + 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(); } /** @@ -1293,7 +1311,7 @@ contract Comet is CometMainInterface { function approveThis(address manager, address asset, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); - asset.call(abi.encodeWithSelector(ERC20.approve.selector, manager, amount)); + asset.call(abi.encodeWithSelector(IERC20NonStandard.approve.selector, manager, amount)); } /** diff --git a/contracts/CometMainInterface.sol b/contracts/CometMainInterface.sol index 1635157b5..5347b22f7 100644 --- a/contracts/CometMainInterface.sol +++ b/contracts/CometMainInterface.sol @@ -30,7 +30,8 @@ abstract contract CometMainInterface is CometCore { error TimestampTooLarge(); error TooManyAssets(); error TooMuchSlippage(); - error TransferFailed(bytes error); + error TransferInFailed(); + error TransferOutFailed(); error Unauthorized(); event Supply(address indexed from, address indexed dst, uint amount); From 9a33adc57867efe5c079b324343f8a77fe8c4857 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 13 Nov 2023 14:31:35 -0800 Subject: [PATCH 21/36] tests update --- test/supply-test.ts | 3 +-- test/withdraw-test.ts | 6 ++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/supply-test.ts b/test/supply-test.ts index 65d9da800..c883fdb8f 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -556,10 +556,9 @@ describe('supplyTo', function () { await comet.connect(alice).allow(EVIL.address, true); await wait(EVIL.connect(alice).approve(comet.address, 75e6)); await EVIL.allocateTo(alice.address, 75e6); - const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0,10); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWithCustomError(comet, 'TransferFailed').withArgs(ReentrantCallBlockedErrorSelector); + ).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked'); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index cd953b89c..7fe2c0f71 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -509,10 +509,9 @@ describe('withdraw', function () { await comet.connect(alice).allow(EVIL.address, true); // In callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) - const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0, 10); await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWithCustomError(comet, 'TransferFailed').withArgs(ReentrantCallBlockedErrorSelector); + ).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked'); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -557,10 +556,9 @@ describe('withdraw', function () { await comet.connect(alice).allow(EVIL.address, true); // in callback, EvilToken attempts to withdraw USDC to bob's address - const ReentrantCallBlockedErrorSelector = ethers.utils.keccak256(ethers.utils.toUtf8Bytes('ReentrantCallBlocked()')).slice(0, 10); await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWithCustomError(comet, 'TransferFailed').withArgs(ReentrantCallBlockedErrorSelector); + ).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked'); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); From f4df8a2103fcfbd1776bc0e535779f1a206dcfc9 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 14 Nov 2023 01:55:18 -0800 Subject: [PATCH 22/36] Add memory safe assembly --- contracts/Comet.sol | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index ed5ec2760..22a39a887 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -784,7 +784,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 @@ -808,7 +808,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 @@ -1310,8 +1310,7 @@ contract Comet is CometMainInterface { */ function approveThis(address manager, address asset, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); - - asset.call(abi.encodeWithSelector(IERC20NonStandard.approve.selector, manager, amount)); + IERC20NonStandard(asset).approve(manager, amount); } /** From d68867203ddb9f02f604202f7b3da3d3721a3647 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 28 Nov 2023 15:05:48 -0800 Subject: [PATCH 23/36] change reentrant to separate functions to reduce space --- contracts/Comet.sol | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 22a39a887..5d7f95cc0 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -202,6 +202,12 @@ contract Comet is CometMainInterface { } modifier nonReentrant() { + nonReentrantBefore(); + _; + nonReentrantAfter(); + } + + function nonReentrantBefore() internal { bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; uint256 status; assembly { @@ -212,7 +218,11 @@ contract Comet is CometMainInterface { assembly { sstore(slot, COMET_REENTRANCY_GUARD_ENTERED) } - _; + } + + function nonReentrantAfter() internal { + bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; + uint256 status; assembly { sstore(slot, COMET_REENTRANCY_GUARD_NOT_ENTERED) } From 99da5c42a1dcdc824b8ab322232163c0beb5025b Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 28 Nov 2023 15:14:35 -0800 Subject: [PATCH 24/36] address comments --- contracts/Comet.sol | 11 ++++++----- contracts/CometCore.sol | 8 ++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 5d7f95cc0..e2ba69456 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -208,23 +208,23 @@ contract Comet is CometMainInterface { } function nonReentrantBefore() internal { - bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; + bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; uint256 status; assembly { status := sload(slot) } - if (status == COMET_REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked(); + if (status == REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked(); assembly { - sstore(slot, COMET_REENTRANCY_GUARD_ENTERED) + sstore(slot, REENTRANCY_GUARD_ENTERED) } } function nonReentrantAfter() internal { - bytes32 slot = COMET_REENTRANCY_GUARD_FLAG_SLOT; + bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; uint256 status; assembly { - sstore(slot, COMET_REENTRANCY_GUARD_NOT_ENTERED) + sstore(slot, REENTRANCY_GUARD_NOT_ENTERED) } } @@ -1320,6 +1320,7 @@ contract Comet is CometMainInterface { */ function approveThis(address manager, address asset, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); + IERC20NonStandard(asset).approve(manager, amount); } diff --git a/contracts/CometCore.sol b/contracts/CometCore.sol index 3ff2ada40..534f2701b 100644 --- a/contracts/CometCore.sol +++ b/contracts/CometCore.sol @@ -57,11 +57,11 @@ abstract contract CometCore is CometConfiguration, CometStorage, CometMath { uint64 internal constant FACTOR_SCALE = 1e18; /// @dev The storage slot for reentrancy guard flags - bytes32 internal constant COMET_REENTRANCY_GUARD_FLAG_SLOT = bytes32(keccak256("comet.reentrancy.guard")); + bytes32 internal constant REENTRANCY_GUARD_FLAG_SLOT = bytes32(keccak256("comet.reentrancy.guard")); - /// @dev The reentrancy guard status - uint256 internal constant COMET_REENTRANCY_GUARD_NOT_ENTERED = 0; - uint256 internal constant COMET_REENTRANCY_GUARD_ENTERED = 1; + /// @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 From 22830d012cda0a21c7b4b40e46d684a0bab28ad1 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 28 Nov 2023 15:26:42 -0800 Subject: [PATCH 25/36] add memory-safe as recommended in other palces --- contracts/Comet.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index e2ba69456..c8985f378 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -210,12 +210,12 @@ contract Comet is CometMainInterface { function nonReentrantBefore() internal { bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; uint256 status; - assembly { + assembly ("memory-safe") { status := sload(slot) } if (status == REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked(); - assembly { + assembly ("memory-safe") { sstore(slot, REENTRANCY_GUARD_ENTERED) } } @@ -223,7 +223,7 @@ contract Comet is CometMainInterface { function nonReentrantAfter() internal { bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; uint256 status; - assembly { + assembly ("memory-safe") { sstore(slot, REENTRANCY_GUARD_NOT_ENTERED) } } From 53376edbd52a785d497873cc1ce82806ec1c1ce7 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 5 Feb 2024 17:19:39 -0800 Subject: [PATCH 26/36] address comments --- contracts/Comet.sol | 9 ++++++ test/buy-collateral-test.ts | 63 +++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/contracts/Comet.sol b/contracts/Comet.sol index c8985f378..4417bd9fe 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -201,12 +201,18 @@ contract Comet is CometMainInterface { (asset14_a, asset14_b) = getPackedAssetInternal(config.assetConfigs, 14); } + /** + * @dev Non-reentrant modifier + */ modifier nonReentrant() { nonReentrantBefore(); _; nonReentrantAfter(); } + /** + * @dev This will set the flag before the call and revert if it is already set + */ function nonReentrantBefore() internal { bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; uint256 status; @@ -220,6 +226,9 @@ contract Comet is CometMainInterface { } } + /** + * @dev This will unset the flag after the call + */ function nonReentrantAfter() internal { bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; uint256 status; diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 75576f13e..7ec220370 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -562,5 +562,68 @@ describe('buyCollateral', function () { // EvilToken attack should be blocked, so totalSupplyBase should be 0 expect(evilBobPortfolio.internal.EVIL).to.equal(0); }); + + it('reentrant supply is reverted', async () => { + 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.SupplyFrom, + source: evilAlice.address, + destination: evilBob.address, + asset: EVIL.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 in callback + await expect(evilComet + .connect(evilAlice) + .buyCollateral( + evilWETH.address, + exp(0, 18), + exp(3000, 6), + evilAlice.address + )).to.be.revertedWith("custom error 'ReentrantCallBlocked()'"); + }); }); }); From 58d1df69dff719bf5bbf959380f72d83be3aedef Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 5 Feb 2024 17:23:49 -0800 Subject: [PATCH 27/36] lint --- test/buy-collateral-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 7ec220370..d5a265d7d 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -623,7 +623,7 @@ describe('buyCollateral', function () { exp(0, 18), exp(3000, 6), evilAlice.address - )).to.be.revertedWith("custom error 'ReentrantCallBlocked()'"); + )).to.be.revertedWith("custom error 'ReentrantCallBlocked()'"); }); }); }); From 7d9dbe55fa5a3ca45a06697baca4affc8755bf95 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Mon, 5 Feb 2024 18:56:33 -0800 Subject: [PATCH 28/36] 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 4417bd9fe..405a6aa1f 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -202,7 +202,7 @@ contract Comet is CometMainInterface { } /** - * @dev Non-reentrant modifier + * @dev Prevents marked functions from being reentered */ modifier nonReentrant() { nonReentrantBefore(); From d45e8fe3d5443743c83da5616148237cd549753b Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Mon, 5 Feb 2024 18:56:39 -0800 Subject: [PATCH 29/36] 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 405a6aa1f..98ea74aa9 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -211,7 +211,7 @@ contract Comet is CometMainInterface { } /** - * @dev This will set the flag before the call and revert if it is already set + * @dev Checks that the reentrancy flag is not set and then sets the flag */ function nonReentrantBefore() internal { bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; From b53c096a125bbea9dec25ce2f62e1ee516caadba Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Mon, 5 Feb 2024 18:56:46 -0800 Subject: [PATCH 30/36] 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 98ea74aa9..77bfaa0fe 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -227,7 +227,7 @@ contract Comet is CometMainInterface { } /** - * @dev This will unset the flag after the call + * @dev Unsets the reentrancy flag */ function nonReentrantAfter() internal { bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT; From 5c10dab9a2f4cf976a1c69dd08515841b8ba2b11 Mon Sep 17 00:00:00 2001 From: Hans Wang <2709448+cwang25@users.noreply.github.com> Date: Mon, 5 Feb 2024 18:56:51 -0800 Subject: [PATCH 31/36] Update test/buy-collateral-test.ts Co-authored-by: Kevin Cheng --- test/buy-collateral-test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index d5a265d7d..4fee8ca8d 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -615,7 +615,7 @@ describe('buyCollateral', function () { // authorize EVIL, since callback will originate from EVIL token address await evilComet.connect(evilAlice).allow(EVIL.address, true); - // call buyCollateral; supplyFrom is called in in callback + // call buyCollateral; supplyFrom is called in callback await expect(evilComet .connect(evilAlice) .buyCollateral( From d077e350a7a74d3d6ddbcd9c5d96b60241a3ef27 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Mon, 5 Feb 2024 19:01:21 -0800 Subject: [PATCH 32/36] address more comments --- test/buy-collateral-test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 4fee8ca8d..86ff8f872 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -563,7 +563,7 @@ describe('buyCollateral', function () { expect(evilBobPortfolio.internal.EVIL).to.equal(0); }); - it('reentrant supply is reverted', async () => { + it('reentrant buyCollateral is reverted', async () => { const wethArgs = { initial: 1e4, decimals: 18, @@ -615,6 +615,7 @@ describe('buyCollateral', function () { // 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) From 23cb021aa827d5d86990dbf0402180398bf5b00e Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 6 Feb 2024 11:08:50 -0800 Subject: [PATCH 33/36] address comment to let new test to test on buyCollateral attacks specifically --- .../1691022234_configurate_and_ens.ts | 206 ++++++++++++++++++ test/buy-collateral-test.ts | 7 +- test/helpers.ts | 3 +- 3 files changed, 212 insertions(+), 4 deletions(-) create mode 100644 deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts diff --git a/deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts b/deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts new file mode 100644 index 000000000..65534f788 --- /dev/null +++ b/deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts @@ -0,0 +1,206 @@ +import { DeploymentManager, migration } from '../../../../plugins/deployment_manager'; +import { exp, getConfigurationStruct, proposal } from '../../../../src/deploy'; +import { diffState, getCometConfig } from '../../../../plugins/deployment_manager/DiffState'; +import { expect } from 'chai'; + +const COMPAddress = '0x3587b2F7E0E2D6166d6C14230e7Fe160252B0ba4'; +const ENSName = 'compound-community-licenses.eth'; +const ENSResolverAddress = '0x19c2d5D0f035563344dBB7bE5fD09c8dad62b001'; +const ENSSubdomainLabel = 'v3-additional-grants'; +const ENSSubdomain = `${ENSSubdomainLabel}.${ENSName}`; +const ENSTextRecordKey = 'v3-official-markets'; +const USDCAmountToSeed = exp(5, 6); + +export default migration('1691022234_configurate_and_ens', { + async prepare(deploymentManager: DeploymentManager) { + const cometFactory = await deploymentManager.deploy('cometFactory', 'CometFactory.sol', [], true); + return { newFactoryAddress: cometFactory.address }; + }, + + enact: async (deploymentManager: DeploymentManager, govDeploymentManager: DeploymentManager, { newFactoryAddress }) => { + const trace = deploymentManager.tracer(); + + // Import shared contracts from cUSDCv3 + + const AllContracts = await deploymentManager.getContracts(); + const { + governor, + comet, + configurator, + cometAdmin, + rewards, + USDT, + } = AllContracts; + + const configuration = await getConfigurationStruct(deploymentManager); + + const ENSResolver = await deploymentManager.existing('ENSResolver', ENSResolverAddress, 'goerli'); + const subdomainHash = ethers.utils.namehash(ENSSubdomain); + const chainId = (await deploymentManager.hre.ethers.provider.getNetwork()).chainId.toString(); + const newMarketObject = { baseSymbol: 'USDT', cometAddress: comet.address }; + const officialMarketsJSON = JSON.parse(await ENSResolver.text(subdomainHash, ENSTextRecordKey)); + if (officialMarketsJSON[chainId]) { + officialMarketsJSON[chainId].push(newMarketObject); + } else { + officialMarketsJSON[chainId] = [newMarketObject]; + } + + const actions = [ + // 1. Set the factory in the Configurator + { + contract: configurator, + signature: 'setFactory(address,address)', + args: [comet.address, newFactoryAddress], + }, + + // 2. Set the configuration in the Configurator + { + contract: configurator, + signature: 'setConfiguration(address,(address,address,address,address,address,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint104,uint104,uint104,(address,address,uint8,uint64,uint64,uint64,uint128)[]))', + args: [comet.address, configuration], + }, + + // 3. Deploy and upgrade to a new version of Comet + { + contract: cometAdmin, + signature: "deployAndUpgradeTo(address,address)", + args: [configurator.address, comet.address], + }, + + // 4. Set the rewards configuration to COMP + { + contract: rewards, + signature: "setRewardConfig(address,address)", + args: [comet.address, COMPAddress], + }, + + // 5. Set the official markets text record on the subdomain + { + target: ENSResolverAddress, + signature: 'setText(bytes32,string,string)', + calldata: ethers.utils.defaultAbiCoder.encode( + ['bytes32', 'string', 'string'], + [subdomainHash, ENSTextRecordKey, JSON.stringify(officialMarketsJSON)] + ) + }, + + // 6. Send USDT from Timelock to Comet + // XXX assert that funds have been transferred by diffing the balances before and after + { + contract: USDT, + signature: "transfer(address,uint256)", + args: [comet.address, exp(20_000_000, 6)], + }, + ]; + + const description = "# Initialize cUSDTv3 on Goerli" + const txn = await deploymentManager.retry( + async () => trace((await governor.propose(...await proposal(actions, description)))) + ); + + const event = txn.events.find(event => event.event === 'ProposalCreated'); + const [proposalId] = event.args; + + trace(`Created proposal ${proposalId}.`); + }, + + async verify(deploymentManager: DeploymentManager, govDeploymentManager: DeploymentManager, preMigrationBlockNumber: number) { + const ethers = deploymentManager.hre.ethers; + await deploymentManager.spider(); // Pull in Arbitrum COMP now that reward config has been set + const { + comet, + rewards, + } = await deploymentManager.getContracts(); + + const config = await rewards.rewardConfig(comet.address); + + // Verify state changes + // const stateChanges = await diffState(comet, getCometConfig, preMigrationBlockNumber); + // TODO: Will uncomment once the comet has been deployed + // expect(stateChanges).to.deep.equal({ + // COMP: { + // supplyCap: exp(500_000, 18) + // }, + // WBTC: { + // supplyCap: exp(35_000, 8) + // }, + // WETH: { + // supplyCap: exp(1_000_000, 18) + // }, + // baseTrackingSupplySpeed: exp(34.74 / 86400, 15, 18), + // baseTrackingBorrowSpeed: exp(34.74 / 86400, 15, 18), + // }); + + expect(config.token).to.be.equal(COMPAddress); + expect(config.rescaleFactor).to.be.equal(exp(1, 12)); + expect(config.shouldUpscale).to.be.equal(true); + + // Verify the seeded USDT reaches Comet reserve + expect(await comet.getReserves()).to.be.equal(exp(20_000_000, 6)); + + // Verify the official markets are updated + const ENSResolver = await deploymentManager.existing('ENSResolver', ENSResolverAddress, 'goerli'); + const subdomainHash = ethers.utils.namehash(ENSSubdomain); + const officialMarkets = JSON.parse(await ENSResolver.text(subdomainHash, ENSTextRecordKey)); + + expect(officialMarkets).to.deep.equal({ + 5: [ + { + baseSymbol: 'USDC', + cometAddress: '0x3EE77595A8459e93C2888b13aDB354017B198188', + }, + { + baseSymbol: 'WETH', + cometAddress: '0x9A539EEc489AAA03D588212a164d0abdB5F08F5F', + }, + { + baseSymbol: 'USDT', + cometAddress: comet.address, + } + ], + + 420: [ + { + baseSymbol: 'USDC', + cometAddress: '0xb8F2f9C84ceD7bBCcc1Db6FB7bb1F19A9a4adfF4' + } + ], + + 421613: [ + { + baseSymbol: 'USDC.e', + cometAddress: '0x1d573274E19174260c5aCE3f2251598959d24456', + }, + { + baseSymbol: 'USDC', + cometAddress: '0x0C94d3F9D7F211630EDecAF085718Ac80821A6cA', + }, + ], + + 59140: [ + { + baseSymbol: 'USDC', + cometAddress: '0xa84b24A43ba1890A165f94Ad13d0196E5fD1023a' + } + ], + + 84531: [ + { + baseSymbol: 'USDC', + cometAddress: '0xe78Fc55c884704F9485EDa042fb91BfE16fD55c1' + }, + { + baseSymbol: 'WETH', + cometAddress: '0xED94f3052638620fE226a9661ead6a39C2a265bE' + } + ], + + 80001: [ + { + baseSymbol: 'USDC', + cometAddress: '0xF09F0369aB0a875254fB565E52226c88f10Bc839' + }, + ] + }); + } +}); \ No newline at end of file diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index 86ff8f872..b0128b9fe 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 blocked during reentrant supply', async () => { + it('is not broken by reentrancy supply', async () => { const wethArgs = { initial: 1e4, decimals: 18, @@ -596,12 +596,13 @@ describe('buyCollateral', function () { // add attack to EVIL token const attack = Object.assign({}, await EVIL.getAttack(), { - attackType: ReentryAttack.SupplyFrom, + attackType: ReentryAttack.BuyCollateral, source: evilAlice.address, destination: evilBob.address, asset: EVIL.address, amount: 3000e6, - maxCalls: 1 + maxCalls: 1, + collateralAsset: evilWETH.address, }); await EVIL.setAttack(attack); 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 = { From dd10703d9267ea901a08b8bc5681a6b83d6ce06f Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 6 Feb 2024 12:12:40 -0800 Subject: [PATCH 34/36] evil token update --- contracts/test/EvilToken.sol | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/contracts/test/EvilToken.sol b/contracts/test/EvilToken.sol index ffb2a1ed5..111243a91 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 { @@ -23,6 +24,7 @@ contract EvilToken is FaucetToken { address asset; uint amount; uint maxCalls; + address collateralAsset; } ReentryAttack public attack; @@ -40,7 +42,8 @@ contract EvilToken is FaucetToken { destination: address(this), asset: address(this), amount: 1e6, - maxCalls: type(uint).max + maxCalls: type(uint).max, + collateralAsset: address(0) }); } @@ -92,6 +95,13 @@ contract EvilToken is FaucetToken { reentryAttack.asset, reentryAttack.amount ); + } else if (reentryAttack.attackType == AttackType.BUY_COLLATERAL) { + Comet(payable(msg.sender)).buyCollateral( + reentryAttack.collateralAsset, + 0, + reentryAttack.amount, + reentryAttack.destination + ); } else { revert("invalid reentry attack"); } From 763f8064c53cf0c809a5cc772b512bdd739921d0 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 6 Feb 2024 17:00:12 -0800 Subject: [PATCH 35/36] remove accidently checked in test file --- .../1691022234_configurate_and_ens.ts | 206 ------------------ 1 file changed, 206 deletions(-) delete mode 100644 deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts diff --git a/deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts b/deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts deleted file mode 100644 index 65534f788..000000000 --- a/deployments/goerli/usdt/migrations/1691022234_configurate_and_ens.ts +++ /dev/null @@ -1,206 +0,0 @@ -import { DeploymentManager, migration } from '../../../../plugins/deployment_manager'; -import { exp, getConfigurationStruct, proposal } from '../../../../src/deploy'; -import { diffState, getCometConfig } from '../../../../plugins/deployment_manager/DiffState'; -import { expect } from 'chai'; - -const COMPAddress = '0x3587b2F7E0E2D6166d6C14230e7Fe160252B0ba4'; -const ENSName = 'compound-community-licenses.eth'; -const ENSResolverAddress = '0x19c2d5D0f035563344dBB7bE5fD09c8dad62b001'; -const ENSSubdomainLabel = 'v3-additional-grants'; -const ENSSubdomain = `${ENSSubdomainLabel}.${ENSName}`; -const ENSTextRecordKey = 'v3-official-markets'; -const USDCAmountToSeed = exp(5, 6); - -export default migration('1691022234_configurate_and_ens', { - async prepare(deploymentManager: DeploymentManager) { - const cometFactory = await deploymentManager.deploy('cometFactory', 'CometFactory.sol', [], true); - return { newFactoryAddress: cometFactory.address }; - }, - - enact: async (deploymentManager: DeploymentManager, govDeploymentManager: DeploymentManager, { newFactoryAddress }) => { - const trace = deploymentManager.tracer(); - - // Import shared contracts from cUSDCv3 - - const AllContracts = await deploymentManager.getContracts(); - const { - governor, - comet, - configurator, - cometAdmin, - rewards, - USDT, - } = AllContracts; - - const configuration = await getConfigurationStruct(deploymentManager); - - const ENSResolver = await deploymentManager.existing('ENSResolver', ENSResolverAddress, 'goerli'); - const subdomainHash = ethers.utils.namehash(ENSSubdomain); - const chainId = (await deploymentManager.hre.ethers.provider.getNetwork()).chainId.toString(); - const newMarketObject = { baseSymbol: 'USDT', cometAddress: comet.address }; - const officialMarketsJSON = JSON.parse(await ENSResolver.text(subdomainHash, ENSTextRecordKey)); - if (officialMarketsJSON[chainId]) { - officialMarketsJSON[chainId].push(newMarketObject); - } else { - officialMarketsJSON[chainId] = [newMarketObject]; - } - - const actions = [ - // 1. Set the factory in the Configurator - { - contract: configurator, - signature: 'setFactory(address,address)', - args: [comet.address, newFactoryAddress], - }, - - // 2. Set the configuration in the Configurator - { - contract: configurator, - signature: 'setConfiguration(address,(address,address,address,address,address,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint64,uint104,uint104,uint104,(address,address,uint8,uint64,uint64,uint64,uint128)[]))', - args: [comet.address, configuration], - }, - - // 3. Deploy and upgrade to a new version of Comet - { - contract: cometAdmin, - signature: "deployAndUpgradeTo(address,address)", - args: [configurator.address, comet.address], - }, - - // 4. Set the rewards configuration to COMP - { - contract: rewards, - signature: "setRewardConfig(address,address)", - args: [comet.address, COMPAddress], - }, - - // 5. Set the official markets text record on the subdomain - { - target: ENSResolverAddress, - signature: 'setText(bytes32,string,string)', - calldata: ethers.utils.defaultAbiCoder.encode( - ['bytes32', 'string', 'string'], - [subdomainHash, ENSTextRecordKey, JSON.stringify(officialMarketsJSON)] - ) - }, - - // 6. Send USDT from Timelock to Comet - // XXX assert that funds have been transferred by diffing the balances before and after - { - contract: USDT, - signature: "transfer(address,uint256)", - args: [comet.address, exp(20_000_000, 6)], - }, - ]; - - const description = "# Initialize cUSDTv3 on Goerli" - const txn = await deploymentManager.retry( - async () => trace((await governor.propose(...await proposal(actions, description)))) - ); - - const event = txn.events.find(event => event.event === 'ProposalCreated'); - const [proposalId] = event.args; - - trace(`Created proposal ${proposalId}.`); - }, - - async verify(deploymentManager: DeploymentManager, govDeploymentManager: DeploymentManager, preMigrationBlockNumber: number) { - const ethers = deploymentManager.hre.ethers; - await deploymentManager.spider(); // Pull in Arbitrum COMP now that reward config has been set - const { - comet, - rewards, - } = await deploymentManager.getContracts(); - - const config = await rewards.rewardConfig(comet.address); - - // Verify state changes - // const stateChanges = await diffState(comet, getCometConfig, preMigrationBlockNumber); - // TODO: Will uncomment once the comet has been deployed - // expect(stateChanges).to.deep.equal({ - // COMP: { - // supplyCap: exp(500_000, 18) - // }, - // WBTC: { - // supplyCap: exp(35_000, 8) - // }, - // WETH: { - // supplyCap: exp(1_000_000, 18) - // }, - // baseTrackingSupplySpeed: exp(34.74 / 86400, 15, 18), - // baseTrackingBorrowSpeed: exp(34.74 / 86400, 15, 18), - // }); - - expect(config.token).to.be.equal(COMPAddress); - expect(config.rescaleFactor).to.be.equal(exp(1, 12)); - expect(config.shouldUpscale).to.be.equal(true); - - // Verify the seeded USDT reaches Comet reserve - expect(await comet.getReserves()).to.be.equal(exp(20_000_000, 6)); - - // Verify the official markets are updated - const ENSResolver = await deploymentManager.existing('ENSResolver', ENSResolverAddress, 'goerli'); - const subdomainHash = ethers.utils.namehash(ENSSubdomain); - const officialMarkets = JSON.parse(await ENSResolver.text(subdomainHash, ENSTextRecordKey)); - - expect(officialMarkets).to.deep.equal({ - 5: [ - { - baseSymbol: 'USDC', - cometAddress: '0x3EE77595A8459e93C2888b13aDB354017B198188', - }, - { - baseSymbol: 'WETH', - cometAddress: '0x9A539EEc489AAA03D588212a164d0abdB5F08F5F', - }, - { - baseSymbol: 'USDT', - cometAddress: comet.address, - } - ], - - 420: [ - { - baseSymbol: 'USDC', - cometAddress: '0xb8F2f9C84ceD7bBCcc1Db6FB7bb1F19A9a4adfF4' - } - ], - - 421613: [ - { - baseSymbol: 'USDC.e', - cometAddress: '0x1d573274E19174260c5aCE3f2251598959d24456', - }, - { - baseSymbol: 'USDC', - cometAddress: '0x0C94d3F9D7F211630EDecAF085718Ac80821A6cA', - }, - ], - - 59140: [ - { - baseSymbol: 'USDC', - cometAddress: '0xa84b24A43ba1890A165f94Ad13d0196E5fD1023a' - } - ], - - 84531: [ - { - baseSymbol: 'USDC', - cometAddress: '0xe78Fc55c884704F9485EDa042fb91BfE16fD55c1' - }, - { - baseSymbol: 'WETH', - cometAddress: '0xED94f3052638620fE226a9661ead6a39C2a265bE' - } - ], - - 80001: [ - { - baseSymbol: 'USDC', - cometAddress: '0xF09F0369aB0a875254fB565E52226c88f10Bc839' - }, - ] - }); - } -}); \ No newline at end of file From 68204dad08e846cb6dd4342cf71a1804942c2b21 Mon Sep 17 00:00:00 2001 From: Hans Wang Date: Tue, 6 Feb 2024 17:05:12 -0800 Subject: [PATCH 36/36] fix last two comments --- contracts/test/EvilToken.sol | 6 ++---- test/buy-collateral-test.ts | 5 ++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/contracts/test/EvilToken.sol b/contracts/test/EvilToken.sol index 111243a91..5bc7826af 100644 --- a/contracts/test/EvilToken.sol +++ b/contracts/test/EvilToken.sol @@ -24,7 +24,6 @@ contract EvilToken is FaucetToken { address asset; uint amount; uint maxCalls; - address collateralAsset; } ReentryAttack public attack; @@ -42,8 +41,7 @@ contract EvilToken is FaucetToken { destination: address(this), asset: address(this), amount: 1e6, - maxCalls: type(uint).max, - collateralAsset: address(0) + maxCalls: type(uint).max }); } @@ -97,7 +95,7 @@ contract EvilToken is FaucetToken { ); } else if (reentryAttack.attackType == AttackType.BUY_COLLATERAL) { Comet(payable(msg.sender)).buyCollateral( - reentryAttack.collateralAsset, + reentryAttack.asset, 0, reentryAttack.amount, reentryAttack.destination diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index b0128b9fe..9628ab6a8 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -599,10 +599,9 @@ describe('buyCollateral', function () { attackType: ReentryAttack.BuyCollateral, source: evilAlice.address, destination: evilBob.address, - asset: EVIL.address, + asset: evilWETH.address, amount: 3000e6, - maxCalls: 1, - collateralAsset: evilWETH.address, + maxCalls: 1 }); await EVIL.setAttack(attack);