BracketX - Bracket.fi

Multiple contracts are using the Initializable module from OpenZeppelin. In order to prevent leaving an implementation contract uninitialized OpenZeppelin's documentation recommends adding the _disableInitializers function in the constructor to lock the contracts automatically when they are deployed:

/**
 * @dev Locks the contract, preventing any future reinitialization. This cannot be part of an initializer call.
 * Calling this in the constructor of a contract will prevent that contract from being initialized or reinitialized
 * to any version. It is recommended to use this to lock implementation contracts that are designed to be called
 * through proxies.
 */
function _disableInitializers() internal virtual {
    require(!_initializing, "Initializable: contract is initializing");
    if (_initialized < type(uint8).max) {
        _initialized = type(uint8).max;
        emit Initialized(type(uint8).max);
    }
}

\color{black} \color{white}

SOLVED: The Bracket.fi team fixed the issue. Added the _disableInitializers() to the following contracts: Bracketxl, Config, Offersx, PricingSequencer, iBNFT.

The PricingSequencer contract contains the getLatestPrice() function to return the latest price from a given Chainlink price feed address. This is intended to be used with USDC pairs, which will return an 8 decimals price which is later on converted to 18 decimals by multiplying it by 1e10. However, a funder can create an offer sending an asset that would return an 18-decimals price; this would lead to the function returning a 28-decimals price.

PricingSequencer.sol

function getLatestPrice(address asset) override public view returns (uint) {
    // TODO: Add the Sequencer offline check when moving to production
    // this is not supported on the testnet

    if (checkSequencerState()) {
        // If the sequencer is down, do not perform any critical operations
        revert("L2 sequencer down: Chainlink feeds are not being updated");
    }

    //        uint80 roundID,
    //            int price,
    //            uint startedAt,
    //            uint timeStamp,
    //            uint80 answeredInRound
    AggregatorV2V3Interface priceFeed = AggregatorV2V3Interface(asset);
    (, int price, ,,) = priceFeed.latestRoundData();
    return uint256(price).mul(1e10);
}

\color{black} \color{white}

SOLVED: The Bracket.fi team fixed the issue by adding code to the setOfferX function in the Offersx contract that checks that oracle uses 8 decimals before setting an offer. Although the code is commented out like oracles, it cannot be tested in local environments. Comments should be removed before deploying to production.

The use of > consumes more gas than !=. There are some cases where both can be used indistinctly, such as in unsigned integers where numbers can't be negative, and as such, there is only a need to check that a number is not 0.

Code Location

Bracketx.sol

• Line 244: assert(v.id > 0);
• Line 376: if (v.addAvailUSDC > 0) {
• Line 419: require((amountETH > 0 || amountUSDC > 0), "BAL");
• Line 420: if (amountETH > 0) {
• Line 424: if (amountUSDC > 0) {

SOLVED: The Bracket.fi team fixed the issue by replacing > with != in the specified code.

The use of postfix operators i++ consume more gas than prefix operators ++i.

Code Location

Bracketx.sol

• Line 397: for (uint tid = autoClaimPtr; tid <= maxId; tid++ ) {
• Line 405: cnt++; Offersx.sol
• Line 71: for (uint8 i; i < 12; i++) {

SOLVED: The Bracket.fi team fixed the issue by replacing the postfix with prefix operators.

Most of the solidity for loops use an uint256 variable counter that increments by 1 and starts at 0. These increments don't need to be checked for over/underflow because the variable will never reach the max capacity of uint256 as it would run out of gas long before that happens.

Code Location

Bracketx.sol

• Line 397: for (uint tid = autoClaimPtr; tid <= maxId; tid++ ) {
• Line 405: cnt++; Offersx.sol
• Line 71: for (uint8 i; i < 12; i++) {

SOLVED: The Bracket.fi team fixed the issue by unchecking increments in for loops.

It has been detected that constructors or initializing functions of many smart contracts are missing address validation. For example:

Bracketx.sol

function initialize(
    address _ibnft,
    address _usdc,
    address _pricing,
    address _offers,
    address _config
) external initializer {
    __Ownable_init();
    __ReentrancyGuard_init();
    __Pausable_init();

    ibnft = _ibnft;
    usdc = _usdc;
    pricing = _pricing;
    offers = _offers;
    config = _config;
    autoClaimPtr = 1; // first nft is 1 not 0.
}

Every input address should be checked not to be zero, especially the ones that could lead to rendering the contract unusable, lock tokens, etc. This is considered a best practice.

Code Location

Bracketx.sol

• Line 85-102: initialize() iBNFT.sol
• Line 40-42: setBrkt() Offersx.sol
• Line 39-46: initialize()

SOLVED: The Bracket.fi team fixed the issue by implementing non-zero address requirements in the initialized function of contracts.