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

Audit fixes to collateral extension #100

Open
wants to merge 6 commits into
base: woof-software/collateral-extension
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .github/workflows/run-forge-tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
run: forge install

- name: Run tests
run: forge test -vvv --via-ir
run: forge test -vvv --via-ir --optimizer-runs 1
env:
ETHERSCAN_KEY: ${{ secrets.ETHERSCAN_KEY }}
SNOWTRACE_KEY: ${{ secrets.SNOWTRACE_KEY }}
Expand All @@ -33,4 +33,4 @@ jobs:

- name: Build Comet with older solc versions
run: |
forge build --contracts contracts/Comet.sol --use solc:0.8.15 --via-ir
forge build --contracts contracts/Comet.sol --use solc:0.8.15 --via-ir --optimizer-runs 1
198 changes: 182 additions & 16 deletions contracts/AssetList.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,34 +20,105 @@
/// @dev The max value for a collateral factor (1)
uint64 internal constant MAX_COLLATERAL_FACTOR = FACTOR_SCALE;

uint256[] internal assets_a;
uint256[] internal assets_b;
uint256 internal immutable asset00_a;
uint256 internal immutable asset00_b;
uint256 internal immutable asset01_a;
uint256 internal immutable asset01_b;
uint256 internal immutable asset02_a;
uint256 internal immutable asset02_b;
uint256 internal immutable asset03_a;
uint256 internal immutable asset03_b;
uint256 internal immutable asset04_a;
uint256 internal immutable asset04_b;
uint256 internal immutable asset05_a;
uint256 internal immutable asset05_b;
uint256 internal immutable asset06_a;
uint256 internal immutable asset06_b;
uint256 internal immutable asset07_a;
uint256 internal immutable asset07_b;
uint256 internal immutable asset08_a;
uint256 internal immutable asset08_b;
uint256 internal immutable asset09_a;
uint256 internal immutable asset09_b;
uint256 internal immutable asset10_a;
uint256 internal immutable asset10_b;
uint256 internal immutable asset11_a;
uint256 internal immutable asset11_b;
uint256 internal immutable asset12_a;
uint256 internal immutable asset12_b;
uint256 internal immutable asset13_a;
uint256 internal immutable asset13_b;
uint256 internal immutable asset14_a;
uint256 internal immutable asset14_b;
uint256 internal immutable asset15_a;
uint256 internal immutable asset15_b;
uint256 internal immutable asset16_a;
uint256 internal immutable asset16_b;
uint256 internal immutable asset17_a;
uint256 internal immutable asset17_b;
uint256 internal immutable asset18_a;
uint256 internal immutable asset18_b;
uint256 internal immutable asset19_a;
uint256 internal immutable asset19_b;
uint256 internal immutable asset20_a;
uint256 internal immutable asset20_b;
uint256 internal immutable asset21_a;
uint256 internal immutable asset21_b;
uint256 internal immutable asset22_a;
uint256 internal immutable asset22_b;
uint256 internal immutable asset23_a;
uint256 internal immutable asset23_b;

/// @notice The number of assets this contract actually supports
uint8 public immutable numAssets;

constructor(CometConfiguration.AssetConfig[] memory assetConfigs) {
uint8 _numAssets = uint8(assetConfigs.length);
numAssets = _numAssets;
uint256[] memory _assets_a = new uint256[](_numAssets);
uint256[] memory _assets_b = new uint256[](_numAssets);
for (uint i = 0; i < _numAssets; ) {
(uint256 asset_a, uint256 asset_b) = getPackedAssetInternal(assetConfigs, i);
_assets_a[i] = asset_a;
_assets_b[i] = asset_b;
unchecked { i++; }
}
assets_a = _assets_a;
assets_b = _assets_b;

(asset00_a, asset00_b) = getPackedAssetInternal(assetConfigs, 0);
(asset01_a, asset01_b) = getPackedAssetInternal(assetConfigs, 1);
(asset02_a, asset02_b) = getPackedAssetInternal(assetConfigs, 2);
(asset03_a, asset03_b) = getPackedAssetInternal(assetConfigs, 3);
(asset04_a, asset04_b) = getPackedAssetInternal(assetConfigs, 4);
(asset05_a, asset05_b) = getPackedAssetInternal(assetConfigs, 5);
(asset06_a, asset06_b) = getPackedAssetInternal(assetConfigs, 6);
(asset07_a, asset07_b) = getPackedAssetInternal(assetConfigs, 7);
(asset08_a, asset08_b) = getPackedAssetInternal(assetConfigs, 8);
(asset09_a, asset09_b) = getPackedAssetInternal(assetConfigs, 9);
(asset10_a, asset10_b) = getPackedAssetInternal(assetConfigs, 10);
(asset11_a, asset11_b) = getPackedAssetInternal(assetConfigs, 11);
(asset12_a, asset12_b) = getPackedAssetInternal(assetConfigs, 12);
(asset13_a, asset13_b) = getPackedAssetInternal(assetConfigs, 13);
(asset14_a, asset14_b) = getPackedAssetInternal(assetConfigs, 14);
(asset15_a, asset15_b) = getPackedAssetInternal(assetConfigs, 15);
(asset16_a, asset16_b) = getPackedAssetInternal(assetConfigs, 16);
(asset17_a, asset17_b) = getPackedAssetInternal(assetConfigs, 17);
(asset18_a, asset18_b) = getPackedAssetInternal(assetConfigs, 18);
(asset19_a, asset19_b) = getPackedAssetInternal(assetConfigs, 19);
(asset20_a, asset20_b) = getPackedAssetInternal(assetConfigs, 20);
(asset21_a, asset21_b) = getPackedAssetInternal(assetConfigs, 21);
(asset22_a, asset22_b) = getPackedAssetInternal(assetConfigs, 22);
(asset23_a, asset23_b) = getPackedAssetInternal(assetConfigs, 23);
}

/**
* @dev Checks and gets the packed asset info for storage
* @dev Checks and gets the packed asset info for storage in 2 variables
* - in first variable, the asset address is stored in the lower 160 bits (address can be interpreted as uint160),
* the borrow collateral factor in the next 16 bits,
* the liquidate collateral factor in the next 16 bits,
* and the liquidation factor in the next 16 bits
* - in the second variable, the price feed address is stored in the lower 160 bits,
* the asset decimals in the next 8 bits,
* and the supply cap in the next 64 bits
* @param assetConfigs The asset configurations
* @param i The index of the asset info to get
* @return The packed asset info
vitalii-woof-software marked this conversation as resolved.
Show resolved Hide resolved
*/
function getPackedAssetInternal(CometConfiguration.AssetConfig[] memory assetConfigs, uint i) internal view returns (uint256, uint256) {
CometConfiguration.AssetConfig memory assetConfig;
if (i < assetConfigs.length) {
assembly {

Check warning on line 121 in contracts/AssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Avoid to use inline assembly. It is acceptable only in rare cases
assetConfig := mload(add(add(assetConfigs, 0x20), mul(i, 0x20)))
}
} else {
Expand Down Expand Up @@ -102,9 +173,104 @@
*/
function getAssetInfo(uint8 i) public view returns (CometCore.AssetInfo memory) {
if (i >= numAssets) revert CometMainInterface.BadAsset();

uint256 word_a = assets_a[i];
uint256 word_b = assets_b[i];
uint256 word_a;
uint256 word_b;
if(i == 0){
word_a = asset00_a;
word_b = asset00_b;
}
if(i == 1){
word_a = asset01_a;
word_b = asset01_b;
}
if(i == 2){
word_a = asset02_a;
word_b = asset02_b;
}
if(i == 3){
word_a = asset03_a;
word_b = asset03_b;
}
if(i == 4){
word_a = asset04_a;
word_b = asset04_b;
}
if(i == 5){
word_a = asset05_a;
word_b = asset05_b;
}
if(i == 6){
word_a = asset06_a;
word_b = asset06_b;
}
if(i == 7){
word_a = asset07_a;
word_b = asset07_b;
}
if(i == 8){
word_a = asset08_a;
word_b = asset08_b;
}
if(i == 9){
word_a = asset09_a;
word_b = asset09_b;
}
if(i == 10){
word_a = asset10_a;
word_b = asset10_b;
}
if(i == 11){
word_a = asset11_a;
word_b = asset11_b;
}
if(i == 12){
word_a = asset12_a;
word_b = asset12_b;
}
if(i == 13){
word_a = asset13_a;
word_b = asset13_b;
}
if(i == 14){
word_a = asset14_a;
word_b = asset14_b;
}
if(i == 15){
word_a = asset15_a;
word_b = asset15_b;
}
if(i == 16){
word_a = asset16_a;
word_b = asset16_b;
}
if(i == 17){
word_a = asset17_a;
word_b = asset17_b;
}
if(i == 18){
word_a = asset18_a;
word_b = asset18_b;
}
if(i == 19){
word_a = asset19_a;
word_b = asset19_b;
}
if(i == 20){
word_a = asset20_a;
word_b = asset20_b;
}
if(i == 21){
word_a = asset21_a;
word_b = asset21_b;
}
if(i == 22){
word_a = asset22_a;
word_b = asset22_b;
}
if(i == 23){
word_a = asset23_a;
word_b = asset23_b;
}

address asset = address(uint160(word_a & type(uint160).max));
uint64 rescale = FACTOR_SCALE / 1e4;
Expand Down
5 changes: 5 additions & 0 deletions contracts/AssetListFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import "./AssetList.sol";
contract AssetListFactory {
event AssetListCreated(address indexed assetList, CometCore.AssetConfig[] assetConfigs);

/**
* @notice Create a new asset list
* @param assetConfigs The asset configurations
* @return assetList The address of the new asset list
*/
function createAssetList(CometCore.AssetConfig[] memory assetConfigs) external returns (address assetList) {
assetList = address(new AssetList(assetConfigs));
emit AssetListCreated(assetList, assetConfigs);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.15;

import "./CometExtendedAssetList.sol";
import "./CometWithExtendedAssetList.sol";
import "./CometConfiguration.sol";

contract CometFactoryExtendedAssetList is CometConfiguration {
contract CometFactoryWithExtendedAssetList is CometConfiguration {
function clone(Configuration calldata config) external returns (address) {
return address(new CometExtendedAssetList(config));
return address(new CometWithExtendedAssetList(config));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* @notice An efficient monolithic money market protocol
* @author Compound
*/
contract CometExtendedAssetList is CometMainInterface {
contract CometWithExtendedAssetList is CometMainInterface {
/** General configuration constants **/

/// @notice The admin of the protocol
Expand Down Expand Up @@ -179,12 +179,12 @@
function nonReentrantBefore() internal {
bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT;
uint256 status;
assembly ("memory-safe") {

Check warning on line 182 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Avoid to use inline assembly. It is acceptable only in rare cases
status := sload(slot)
}

if (status == REENTRANCY_GUARD_ENTERED) revert ReentrantCallBlocked();
assembly ("memory-safe") {

Check warning on line 187 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Avoid to use inline assembly. It is acceptable only in rare cases
sstore(slot, REENTRANCY_GUARD_ENTERED)
}
}
Expand All @@ -194,8 +194,8 @@
*/
function nonReentrantAfter() internal {
bytes32 slot = REENTRANCY_GUARD_FLAG_SLOT;
uint256 status;

Check warning on line 197 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Variable "status" is unused
assembly ("memory-safe") {

Check warning on line 198 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Avoid to use inline assembly. It is acceptable only in rare cases
sstore(slot, REENTRANCY_GUARD_NOT_ENTERED)
}
}
Expand Down Expand Up @@ -660,7 +660,7 @@
uint256 preTransferBalance = IERC20NonStandard(asset).balanceOf(address(this));
IERC20NonStandard(asset).transferFrom(from, address(this), amount);
bool success;
assembly ("memory-safe") {

Check warning on line 663 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Avoid to use inline assembly. It is acceptable only in rare cases
switch returndatasize()
case 0 { // This is a non-standard ERC-20
success := not(0) // set success to true
Expand All @@ -684,7 +684,7 @@
function doTransferOut(address asset, address to, uint amount) internal {
IERC20NonStandard(asset).transfer(to, amount);
bool success;
assembly ("memory-safe") {

Check warning on line 687 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Avoid to use inline assembly. It is acceptable only in rare cases
switch returndatasize()
case 0 { // This is a non-standard ERC-20
success := not(0) // set success to true
Expand Down Expand Up @@ -1239,9 +1239,9 @@
/**
* @notice Fallback to calling the extension delegate for everything else
*/
fallback() external payable {

Check warning on line 1242 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Fallback function must be simple
address delegate = extensionDelegate;
assembly {

Check warning on line 1244 in contracts/CometWithExtendedAssetList.sol

View workflow job for this annotation

GitHub Actions / Contract linter

Avoid to use inline assembly. It is acceptable only in rare cases
calldatacopy(0, 0, calldatasize())
let result := delegatecall(gas(), delegate, 0, calldatasize(), 0, 0)
returndatacopy(0, 0, returndatasize())
Expand Down
1 change: 1 addition & 0 deletions contracts/IAssetList.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ import "./CometCore.sol";
*/
interface IAssetList {
function getAssetInfo(uint8 i) external view returns (CometCore.AssetInfo memory);
function numAssets() external view returns (uint8);
}
5 changes: 5 additions & 0 deletions contracts/IAssetListFactory.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,10 @@ import "./CometCore.sol";
* @author Compound
*/
interface IAssetListFactory {
/**
* @notice Create a new asset list
* @param assetConfigs The asset configurations
* @return assetList The address of the new asset list
*/
function createAssetList(CometCore.AssetConfig[] memory assetConfigs) external returns (address assetList);
}
4 changes: 4 additions & 0 deletions contracts/IAssetListFactoryHolder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,9 @@ pragma solidity 0.8.15;
* @author Compound
*/
interface IAssetListFactoryHolder {
/**
* @notice Get the asset list factory
* @return assetListFactory The asset list factory address
*/
function assetListFactory() external view returns (address);
}
6 changes: 3 additions & 3 deletions contracts/test/CometHarnessExtendedAssetList.sol
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.15;

import "../CometExtendedAssetList.sol";
import "../CometWithExtendedAssetList.sol";

contract CometHarnessExtendedAssetList is CometExtendedAssetList {
contract CometHarnessExtendedAssetList is CometWithExtendedAssetList {
uint public nowOverride;

constructor(Configuration memory config) CometExtendedAssetList(config) {}
constructor(Configuration memory config) CometWithExtendedAssetList(config) {}

function getNowInternal() override internal view returns (uint40) {
return nowOverride > 0 ? uint40(nowOverride) : super.getNowInternal();
Expand Down
2 changes: 1 addition & 1 deletion test/absorb-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,7 @@ describe('absorb', function () {
},
reward: 'COMP',
});
const { cometExtendedAssetList : comet, tokens: {
const { cometWithExtendedAssetList : comet, tokens: {
COMP,
WETH,
}, users: [absorber, underwater] } = protocol;
Expand Down
6 changes: 3 additions & 3 deletions test/asset-info-test-asset-list-comet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expect, exp, makeConfigurator, ONE, makeProtocol } from './helpers';

describe('asset info', function () {
it('initializes protocol', async () => {
const { cometExtendedAssetList: comet, tokens } = await makeConfigurator({
const { cometWithExtendedAssetList: comet, tokens } = await makeConfigurator({
assets: {
USDC: {},
ASSET1: {},
Expand Down Expand Up @@ -68,8 +68,8 @@ describe('asset info', function () {
});

it('reverts if index is greater than numAssets', async () => {
const { cometExtendedAssetList } = await makeConfigurator();
await expect(cometExtendedAssetList.getAssetInfo(3)).to.be.revertedWith("custom error 'BadAsset()'");
const { cometWithExtendedAssetList } = await makeConfigurator();
await expect(cometWithExtendedAssetList.getAssetInfo(3)).to.be.revertedWith("custom error 'BadAsset()'");
});

it('reverts if collateral factors are out of range', async () => {
Expand Down
4 changes: 2 additions & 2 deletions test/bulker-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe('bulker', function () {
},
reward: 'COMP',
});
const { cometExtendedAssetList : comet, tokens: {
const { cometWithExtendedAssetList : comet, tokens: {
COMP,
WETH,
USDC,
Expand Down Expand Up @@ -392,7 +392,7 @@ describe('bulker', function () {
},
reward: 'COMP',
});
const { cometExtendedAssetList : comet, tokens: {
const { cometWithExtendedAssetList : comet, tokens: {
COMP,
WETH,
}, users: [alice] } = protocol;
Expand Down
Loading
Loading