Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address OZ's feedback on USDT comet support #818

Merged
merged 36 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
369b363
address L-02
Oct 29, 2023
f7bf99a
use ethcall instead of assembly
Oct 29, 2023
3544043
using low level call, so won't need IERC20Nonstandard as oppose for E…
Oct 30, 2023
85be018
add reentrancy guard, and updated tests
Nov 8, 2023
b2f8d03
address comments
Nov 8, 2023
2c72dff
inline nonReentrant call
Nov 8, 2023
499dce8
update on tests
Nov 8, 2023
3d7fd01
intent
Nov 8, 2023
79ac88a
Update test/buy-collateral-test.ts
cwang25 Nov 9, 2023
5ef9cfd
Update test/supply-test.ts
cwang25 Nov 9, 2023
6a936d1
Update test/withdraw-test.ts
cwang25 Nov 9, 2023
b3dbaa3
Update test/withdraw-test.ts
cwang25 Nov 9, 2023
ad99f2e
address comments
Nov 9, 2023
f2707f2
add low level support to approveThis as well
Nov 9, 2023
9ff7660
add bytes returndata to TransferOut/In() error, remove duplicated tests
Nov 9, 2023
41ee853
address comments, but now getting contract oversize :( both big cont…
Nov 9, 2023
8856e82
trim down a bit on contract size
Nov 10, 2023
e6d6a86
fix tests
Nov 10, 2023
9f770b4
fix lint
Nov 10, 2023
46896c9
use assembly for transfer action, as it save more spaces, and it's al…
Nov 13, 2023
9a33adc
tests update
Nov 13, 2023
f4df8a2
Add memory safe assembly
Nov 14, 2023
d688672
change reentrant to separate functions to reduce space
Nov 28, 2023
99da5c4
address comments
Nov 28, 2023
22830d0
add memory-safe as recommended in other palces
Nov 28, 2023
53376ed
address comments
Feb 6, 2024
58d1df6
lint
Feb 6, 2024
7d9dbe5
Update contracts/Comet.sol
cwang25 Feb 6, 2024
d45e8fe
Update contracts/Comet.sol
cwang25 Feb 6, 2024
b53c096
Update contracts/Comet.sol
cwang25 Feb 6, 2024
5c10dab
Update test/buy-collateral-test.ts
cwang25 Feb 6, 2024
d077e35
address more comments
Feb 6, 2024
23cb021
address comment to let new test to test on buyCollateral attacks spec…
Feb 6, 2024
dd10703
evil token update
Feb 6, 2024
763f806
remove accidently checked in test file
Feb 7, 2024
68204da
fix last two comments
Feb 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions contracts/Comet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,33 @@ contract Comet is CometMainInterface {
(asset14_a, asset14_b) = getPackedAssetInternal(config.assetConfigs, 14);
}

modifier nonReentrant() {
nonReentrantBefore();
_;
nonReentrantAfter();
}

function nonReentrantBefore() internal {
bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT;
uint256 status;
assembly ("memory-safe") {
status := sload(slot)
}

if (status == REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked();
assembly ("memory-safe") {
sstore(slot, REENTRANCY_GUARD_ENTERED)
}
}

function nonReentrantAfter() internal {
bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT;
uint256 status;
assembly ("memory-safe") {
sstore(slot, REENTRANCY_GUARD_NOT_ENTERED)
}
}

/**
* @notice Initialize storage for the contract
* @dev Can be used from constructor or proxy
Expand Down Expand Up @@ -767,7 +794,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
Expand All @@ -791,7 +818,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
Expand Down Expand Up @@ -841,7 +868,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();

Expand Down Expand Up @@ -952,7 +979,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();
Expand Down Expand Up @@ -1063,7 +1090,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();

Expand Down Expand Up @@ -1224,7 +1251,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();
Expand Down Expand Up @@ -1286,6 +1313,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
Expand Down
7 changes: 7 additions & 0 deletions contracts/CometCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ abstract contract CometCore is CometConfiguration, CometStorage, CometMath {
/// @dev The scale for factors
uint64 internal constant FACTOR_SCALE = 1e18;

/// @dev The storage slot for reentrancy guard flags
bytes32 internal constant REENTRANCY_GUARD_FLAG_SLOT = bytes32(keccak256("comet.reentrancy.guard"));

/// @dev The reentrancy guard statuses
uint256 internal constant REENTRANCY_GUARD_NOT_ENTERED = 0;
uint256 internal constant REENTRANCY_GUARD_ENTERED = 1;

/**
* @notice Determine if the manager has permission to act on behalf of the owner
* @param owner The owner account
Expand Down
1 change: 1 addition & 0 deletions contracts/CometMainInterface.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ abstract contract CometMainInterface is CometCore {
error NotForSale();
error NotLiquidatable();
error Paused();
error ReentrantCallBlocked();
error SupplyCapExceeded();
error TimestampTooLarge();
error TooManyAssets();
Expand Down
30 changes: 29 additions & 1 deletion contracts/IERC20NonStandard.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,37 @@ pragma solidity 0.8.15;
* See https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca
*/
interface IERC20NonStandard {
function name() external view returns (string memory);
function symbol() external view returns (string memory);
function decimals() external view returns (uint8);

/**
* @notice Approve `spender` to transfer up to `amount` from `src`
* @dev This will overwrite the approval amount for `spender`
* and is subject to issues noted [here](https://eips.ethereum.org/EIPS/eip-20#approve)
* @param spender The address of the account which may transfer tokens
* @param amount The number of tokens that are approved (-1 means infinite)
*/
function approve(address spender, uint256 amount) external;

/**
* @notice Transfer `value` tokens from `msg.sender` to `to`
* @param to The address of the destination account
* @param value The number of tokens to transfer
*/
function transfer(address to, uint256 value) external;

/**
* @notice Transfer `value` tokens from `from` to `to`
* @param from The address of the source account
* @param to The address of the destination account
* @param value The number of tokens to transfer
*/
function transferFrom(address from, address to, uint256 value) external;

/**
* @notice Gets the balance of the specified address
* @param account The address from which the balance will be retrieved
*/
function balanceOf(address account) external view returns (uint256);
}
}
10 changes: 5 additions & 5 deletions test/buy-collateral-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ describe('buyCollateral', function () {
});

describe('reentrancy', function() {
it('is not broken by reentrancy supply ', async () => {
it('is blocked during reentrant supply', async () => {
const wethArgs = {
initial: 1e4,
decimals: 18,
Expand Down Expand Up @@ -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);
Expand All @@ -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);
});
});
});
54 changes: 7 additions & 47 deletions test/supply-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -524,10 +524,10 @@ describe('supplyTo', function () {
expect(q1.external).to.be.deep.equal({ USDC: 0n, COMP: 0n, WETH: 0n, WBTC: 0n, FeeToken: 0n });
expect(t1.totalSupplyAsset).to.be.equal(t0.totalSupplyAsset.add(1998e8));
// Fee Token logics will cost a bit more gas than standard ERC20 token with no fee calculation
expect(Number(s0.receipt.gasUsed)).to.be.lessThan(163000);
expect(Number(s0.receipt.gasUsed)).to.be.lessThan(186000);
});

it('prevents exceeding the supply cap via re-entrancy', async () => {
it('blocks reentrancy from exceeding the supply cap', async () => {
const { comet, tokens, users: [alice, bob] } = await makeProtocol({
assets: {
USDC: {
Expand Down Expand Up @@ -558,47 +558,7 @@ describe('supplyTo', function () {
await EVIL.allocateTo(alice.address, 75e6);
await expect(
comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6)
).to.be.revertedWith("custom error 'SupplyCapExceeded()'");
});

// This test is purposely test the edge case where comet internal accounting will be incorrect when EvilToken re-entrancy attack
// is performed. The attack will cause comet to have inflated supply amount of token.
//
// We decided that this won't be a concern for Comet, as in order to pull off this attack, the governance has to propose and vote to
// add suspicious token in to Comet's market. As long as governance doesn't add suspicious token contract or erc-777 token to market, the
// Comet should not be vulnerable to this type of attack.
it('incorrect accounting via re-entrancy', async () => {
const { comet, tokens, users: [alice, bob] } = await makeProtocol({
assets: {
USDC: {
decimals: 6
},
EVIL: {
decimals: 6,
initialPrice: 2,
factory: await ethers.getContractFactory('EvilToken') as EvilToken__factory,
supplyCap: 250e6
}
}
});
const { EVIL } = <{ EVIL: EvilToken }>tokens;

const attack = Object.assign({}, await EVIL.getAttack(), {
attackType: ReentryAttack.SupplyFrom,
source: alice.address,
destination: bob.address,
asset: EVIL.address,
amount: 75e6,
maxCalls: 1
});
await EVIL.setAttack(attack);

await comet.connect(alice).allow(EVIL.address, true);
await wait(EVIL.connect(alice).approve(comet.address, 75e6));
await EVIL.allocateTo(alice.address, 75e6);
await comet.connect(alice).supplyTo(bob.address, EVIL.address, 75e6);
// Re-entrancy attack 2 loops, expect to have 2X accounting balance 75*2 = 150
expect(await comet.collateralBalanceOf(bob.address, EVIL.address)).to.be.equal(150e6);
).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked');
});
});

Expand Down
18 changes: 9 additions & 9 deletions test/withdraw-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -475,7 +475,7 @@ describe('withdraw', function () {
});

describe('reentrancy', function () {
it('is not broken by malicious reentrancy transferFrom', async () => {
it('blocks malicious reentrant transferFrom', async () => {
const { comet, tokens, users: [alice, bob] } = await makeProtocol({
assets: {
USDC: {
Expand Down Expand Up @@ -508,10 +508,10 @@ describe('withdraw', function () {
await comet.setCollateralBalance(alice.address, EVIL.address, exp(1, 6));
await comet.connect(alice).allow(EVIL.address, true);

// in callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6)
// In callback, EVIL token calls transferFrom(alice.address, bob.address, 1e6)
await expect(
comet.connect(alice).withdraw(EVIL.address, 1e6)
).to.be.revertedWith("custom error 'NotCollateralized()'");
).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked');

// no USDC transferred
expect(await USDC.balanceOf(comet.address)).to.eq(100e6);
Expand All @@ -521,7 +521,7 @@ describe('withdraw', function () {
expect(await USDC.balanceOf(bob.address)).to.eq(0);
});

it('is not broken by malicious reentrancy withdrawFrom', async () => {
it('blocks malicious reentrant withdrawFrom', async () => {
const { comet, tokens, users: [alice, bob] } = await makeProtocol({
assets: {
USDC: {
Expand Down Expand Up @@ -558,7 +558,7 @@ describe('withdraw', function () {
// in callback, EvilToken attempts to withdraw USDC to bob's address
await expect(
comet.connect(alice).withdraw(EVIL.address, 1e6)
).to.be.revertedWith("custom error 'NotCollateralized()'");
).to.be.revertedWithCustomError(comet, 'ReentrantCallBlocked');

// no USDC transferred
expect(await USDC.balanceOf(comet.address)).to.eq(100e6);
Expand Down