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 1 commit
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
21 changes: 11 additions & 10 deletions contracts/Comet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity 0.8.15;

import "./CometMainInterface.sol";
import "./IERC20NonStandard.sol";
import "./ERC20.sol";
import "./IPriceFeed.sol";

/**
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -482,15 +482,15 @@ 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;
}

/**
* @notice Gets the total amount of protocol reserves of the base asset
*/
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_);
Expand Down Expand Up @@ -764,20 +764,20 @@ 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)))) {
cwang25 marked this conversation as resolved.
Show resolved Hide resolved
revert TransferInFailed();
}
return IERC20NonStandard(asset).balanceOf(address(this)) - preTransferBalance;
return ERC20(asset).balanceOf(address(this)) - preTransferBalance;
}

/**
* @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 {
(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();
cwang25 marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down Expand Up @@ -1262,14 +1262,15 @@ 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
*/
function approveThis(address manager, address asset, uint amount) override external {
if (msg.sender != governor) revert Unauthorized();

cwang25 marked this conversation as resolved.
Show resolved Hide resolved
IERC20NonStandard(asset).approve(manager, amount);
ERC20(asset).approve(manager, amount);
cwang25 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
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 {
kevincheng96 marked this conversation as resolved.
Show resolved Hide resolved
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);
}
}
Loading