diff --git a/contracts/Comet.sol b/contracts/Comet.sol index 2579b92c9..063b05a31 100644 --- a/contracts/Comet.sol +++ b/contracts/Comet.sol @@ -4,14 +4,13 @@ pragma solidity 0.8.15; import "./CometMainInterface.sol"; import "./IERC20NonStandard.sol"; import "./IPriceFeed.sol"; -import "./ReentrancyGuard.sol"; /** * @title Compound's Comet Contract * @notice An efficient monolithic money market protocol * @author Compound */ -contract Comet is CometMainInterface, ReentrancyGuard { +contract Comet is CometMainInterface { /** General configuration constants **/ /// @notice The admin of the protocol @@ -842,7 +841,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @dev Supply either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will repay all of `dst`'s accrued base borrow balance */ - function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal nonReentrant { + function supplyInternal(address operator, address from, address dst, address asset, uint amount) internal { if (isSupplyPaused()) revert Paused(); if (!hasPermission(from, operator)) revert Unauthorized(); @@ -953,7 +952,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @dev Transfer either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will transfer all of `src`'s accrued base balance */ - function transferInternal(address operator, address src, address dst, address asset, uint amount) internal nonReentrant { + function transferInternal(address operator, address src, address dst, address asset, uint amount) internal { if (isTransferPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); if (src == dst) revert NoSelfTransfer(); @@ -1064,7 +1063,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @dev Withdraw either collateral or base asset, depending on the asset, if operator is allowed * @dev Note: Specifying an `amount` of uint256.max will withdraw all of `src`'s accrued base balance */ - function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal nonReentrant { + function withdrawInternal(address operator, address src, address to, address asset, uint amount) internal { if (isWithdrawPaused()) revert Paused(); if (!hasPermission(src, operator)) revert Unauthorized(); @@ -1225,7 +1224,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @param baseAmount The amount of base tokens used to buy the collateral * @param recipient The recipient address */ - function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external nonReentrant { + function buyCollateral(address asset, uint minAmount, uint baseAmount, address recipient) override external { if (isBuyPaused()) revert Paused(); int reserves = getReserves(); @@ -1271,7 +1270,7 @@ contract Comet is CometMainInterface, ReentrancyGuard { * @param to An address of the receiver of withdrawn reserves * @param amount The amount of reserves to be withdrawn from the protocol */ - function withdrawReserves(address to, uint amount) override external nonReentrant { + function withdrawReserves(address to, uint amount) override external { if (msg.sender != governor) revert Unauthorized(); int reserves = getReserves(); diff --git a/contracts/ReentrancyGuard.sol b/contracts/ReentrancyGuard.sol deleted file mode 100644 index af2b98658..000000000 --- a/contracts/ReentrancyGuard.sol +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.9.0) (security/ReentrancyGuard.sol) - -pragma solidity ^0.8.0; - -/** - * @dev Contract module that helps prevent reentrant calls to a function. - * - * Inheriting from `ReentrancyGuard` will make the {nonReentrant} modifier - * available, which can be applied to functions to make sure there are no nested - * (reentrant) calls to them. - * - * Note that because there is a single `nonReentrant` guard, functions marked as - * `nonReentrant` may not call one another. This can be worked around by making - * those functions `private`, and then adding `external` `nonReentrant` entry - * points to them. - * - * TIP: If you would like to learn more about reentrancy and alternative ways - * to protect against it, check out our blog post - * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. - */ -abstract contract ReentrancyGuard { - // Booleans are more expensive than uint256 or any type that takes up a full - // word because each write operation emits an extra SLOAD to first read the - // slot's contents, replace the bits taken up by the boolean, and then write - // back. This is the compiler's defense against contract upgrades and - // pointer aliasing, and it cannot be disabled. - - // The values being non-zero value makes deployment a bit more expensive, - // but in exchange the refund on every call to nonReentrant will be lower in - // amount. Since refunds are capped to a percentage of the total - // transaction's gas, it is best to keep them low in cases like this one, to - // increase the likelihood of the full refund coming into effect. - uint256 private constant _NOT_ENTERED = 1; - uint256 private constant _ENTERED = 2; - - uint256 private _status; - - constructor() { - _status = _NOT_ENTERED; - } - - /** - * @dev Prevents a contract from calling itself, directly or indirectly. - * Calling a `nonReentrant` function from another `nonReentrant` - * function is not supported. It is possible to prevent this from happening - * by making the `nonReentrant` function external, and making it call a - * `private` function that does the actual work. - */ - modifier nonReentrant() { - _nonReentrantBefore(); - _; - _nonReentrantAfter(); - } - - function _nonReentrantBefore() private { - // On the first call to nonReentrant, _status will be _NOT_ENTERED - require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); - - // Any calls to nonReentrant after this point will fail - _status = _ENTERED; - } - - function _nonReentrantAfter() private { - // By storing the original value once again, a refund is triggered (see - // https://eips.ethereum.org/EIPS/eip-2200) - _status = _NOT_ENTERED; - } - - /** - * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates there is a - * `nonReentrant` function in the call stack. - */ - function _reentrancyGuardEntered() internal view returns (bool) { - return _status == _ENTERED; - } -} \ No newline at end of file diff --git a/test/buy-collateral-test.ts b/test/buy-collateral-test.ts index c659258ce..7b6fa7fd0 100644 --- a/test/buy-collateral-test.ts +++ b/test/buy-collateral-test.ts @@ -543,8 +543,8 @@ describe('buyCollateral', function () { expect(normalTotalsBasic.trackingSupplyIndex).to.equal(evilTotalsBasic.trackingSupplyIndex); expect(normalTotalsBasic.trackingBorrowIndex).to.equal(evilTotalsBasic.trackingBorrowIndex); expect(normalTotalsBasic.totalSupplyBase).to.equal(1e6); - // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name) - expect(evilTotalsBasic.totalSupplyBase).to.equal(0e6); + // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should have 3000e6 + expect(evilTotalsBasic.totalSupplyBase).to.equal(3000e6); expect(normalTotalsBasic.totalBorrowBase).to.equal(evilTotalsBasic.totalBorrowBase); expect(normalTotalsCollateral.totalSupplyAsset).to.eq(evilTotalsCollateral.totalSupplyAsset); @@ -559,8 +559,8 @@ describe('buyCollateral', function () { const evilBobPortfolio = await portfolio(evilProtocol, evilBob.address); expect(normalBobPortfolio.internal.USDC).to.equal(1e6); - // EvilToken attack will be blocked by re-entrancy guard, so totalSupplyBase should be 0e6 (under Bob's name) - expect(evilBobPortfolio.internal.EVIL).to.equal(0e6); + // EvilToken attack is using 3000 EVIL tokens, so totalSupplyBase should be 3000e6 (under Bob's name) + expect(evilBobPortfolio.internal.EVIL).to.equal(3000e6); }); }); }); diff --git a/test/supply-test.ts b/test/supply-test.ts index 1e1283a73..fb0e5c976 100644 --- a/test/supply-test.ts +++ b/test/supply-test.ts @@ -466,7 +466,7 @@ describe('supplyTo', function () { expect(t1.totalSupplyBase).to.be.equal(t0.totalSupplyBase.add(999e6)); expect(t1.totalBorrowBase).to.be.equal(t0.totalBorrowBase); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(131000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(128000); }); it('supplies collateral the correct amount in a fee-like situation', async () => { @@ -524,7 +524,7 @@ describe('supplyTo', function () { expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n }); expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(1998e8)); // Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation - expect(Number(s0.receipt.gasUsed)).to.be.lessThan(166000); + expect(Number(s0.receipt.gasUsed)).to.be.lessThan(163000); }); it('prevents exceeding the supply cap via re-entrancy', async () => { @@ -558,7 +558,47 @@ describe('supplyTo', function () { await EVIL.allocateTo(alice.address, 75e6); await expect( comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'SupplyCapExceeded()'"); + }); + + // This test is purposely test the edge case where comet internal accounting will be incorrect when EvilToken re-entrancy attack + // is performed. The attack will cause comet to have inflated supply amount of token. + // + // We decided that this won't be a concern for Comet, as in order to pull off this attack, the governance has to propose and vote to + // add suspicious token in to Comet's market. As long as governance doesn't add suspicious token contract or erc-777 token to market, the + // Comet should not be vulnerable to this type of attack. + it('incorrect accounting via re-entrancy', async () => { + const { comet, tokens, users: [alice, bob] } = await makeProtocol({ + assets: { + USDC: { + decimals: 6 + }, + EVIL: { + decimals: 6, + initialPrice: 2, + factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory, + supplyCap: 250e6 + } + } + }); + const { EVIL } = <{ EVIL: EvilToken }>tokens; + + const attack = Object.assign({}, await EVIL.getAttack(), { + attackType: ReentryAttack.SupplyFrom, + source: alice.address, + destination: bob.address, + asset: EVIL.address, + amount: 75e6, + maxCalls: 1 + }); + await EVIL.setAttack(attack); + + await comet.connect(alice).allow(EVIL.address, true); + await wait(EVIL.connect(alice).approve(comet.address, 75e6)); + await EVIL.allocateTo(alice.address, 75e6); + await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6); + // Re-entrancy attack 2 loops, expect to have 2X accounting balance 75*2 = 150 + expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.equal(150e6); }); }); diff --git a/test/withdraw-test.ts b/test/withdraw-test.ts index 49eabc155..a28d8735b 100644 --- a/test/withdraw-test.ts +++ b/test/withdraw-test.ts @@ -511,7 +511,7 @@ describe('withdraw', function () { // in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6) await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'NotCollateralized()'"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6); @@ -558,7 +558,7 @@ describe('withdraw', function () { // in callback, EvilToken attempts to withdraw USDC to bob's address await expect( comet.connect(alice).withdraw(EVIL.address, 1e6) - ).to.be.revertedWith("ReentrancyGuard: reentrant call"); + ).to.be.revertedWith("custom error 'NotCollateralized()'"); // no USDC transferred expect(await USDC.balanceOf(comet.address)).to.eq(100e6);