Gains Trade

In the GTokenOpenPnlFeed contract, the makeOpenPnlRequest() function is used to send requests to a pool of Chainlink oracles:

GTokenOpenPnlFeed.sol

// Create requests
function makeOpenPnlRequest() private{
    Chainlink.Request memory linkRequest = buildChainlinkRequest(
        job,
        address(this),
        this.fulfill.selector
    );

    uint linkFeePerNode = IERC20(chainlinkTokenAddress())
        .balanceOf(address(this))
        / LINK_FEE_BALANCE_DIVIDER
        / oracles.length;

    requests[++lastRequestId] = Request({
        initiated: true,
        active: true,
        linkFeePerNode: linkFeePerNode
    });

    nextEpochValuesRequestCount++;
    nextEpochValuesLastRequest = block.timestamp;

    for(uint i; i < oracles.length; i ++){
        requestIds[sendChainlinkRequestTo(
            oracles[i],
            linkRequest,
            linkFeePerNode
        )] = lastRequestId;
    }

    emit NextEpochValueRequested(
        gToken.currentEpoch(),
        lastRequestId,
        job,
        oracles.length,
        linkFeePerNode
    );
}

\color{black} \color{white}

The linkFeePerNode variable is wrongly calculated, as it depends on the contract's balance. If the Link balance of the GTokenOpenPnlFeed contract is too high, more Link than needed would be sent to the Chainlink oracles as fees. In case that the Link balance of the GTokenOpenPnlFeed contract is not high enough, the contract will not send enough Link to the Chainlink oracles and the transactions will revert.

RISK ACCEPTED: The Gains Trade team accepted the risk and stated: "We will send enough to the contract so that divided by 1000 it represents the amount we want, and then we will refill it every few months. This pattern allows everyone to send a link to the contract in a decentralized manner. It also means the transaction can never revert, unlike what the issue says because it can never run out of LINK."

Contracts in-scope are missing address validation in some functions. It is possible to configure the zero address, which may cause issues during the contract execution.

Code Location

The following function is not validating, that the given addresses in the newValues array are different from zero:

GTokenOpenPnlFeed.sol

    function updateOracles(address[] memory newValues) external onlyOwner{
        require(newValues.length >= minAnswers, "ARRAY_TOO_SMALL");

        oracles = newValues;
        emit OraclesUpdated(newValues);
    }

The constructor is not validating that the address of the _token argument is different from zero:

TWAPPriceGetter.sol

    constructor(IUniswapV3Pool _pool, address _token, uint32 _twapInterval, uint _precision){
        require(address(_pool) != address(0) && _twapInterval > 0 && _precision > 0, "WRONG_TWAP_CONSTRUCTOR");

        pool = _pool;
        twapInterval = _twapInterval;
        precision = _precision;

        isGnsToken0InLp = pool.token0() == _token;
    }

RISK ACCEPTED: The Gains Trade team accepted the risk of this finding.

The current ownership transfer process for the GToken contract inheriting from Ownable involves the current owner/timelock contract, calling the transferOwnership function.

If the nominated account is not a valid account, it is entirely possible that the owner may accidentally transfer ownership to an uncontrolled account, losing the access to all functions with the onlyOwner modifier.

RISK ACCEPTED: The Gains Trade team accepted the risk of this finding.

In all the loops, the counter variable is incremented using i++. In loops, using ++i costs less gas per iteration than i++.

Code Location

GTokenOpenPnlFeed.sol

• Line 197: for(uint i = 0; i < reqToResetCount; i++)
• Line 248: for(uint i = 0; i < oracles.length; i ++)
• Line 368: for(uint i = 0; i < array.length; i++)

For example, based in the following test contract:

Test.sol

//SPDX-License-Identifier: MIT
pragma solidity 0.8.9;

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

The difference in the gas costs:

ACKNOWLEDGED: The Gains Trade team acknowledged this finding.

As the variable is a uint, it is already initialized to 0. For example, uint256 i = 0 reassigns the 0 to i, which wastes gas.

Code Location

GTokenOpenPnlFeed.sol

• Line 197: for(uint i = 0; i < reqToResetCount; i++)
• Line 248: for(uint i = 0; i < oracles.length; i ++
• Line 368: for(uint i = 0; i < array.length; i++)

SOLVED: The Gains Trade team solved the issue by reducing the gas costs in the Commit ID: f1bcbb4da9efdd8a8be2d28a297cfc9b999d2e1a

Failed operations in the smart contracts in scope are reverted with accompanying messages selected from a set of hard-coded strings.

In the EVM, emitting a hard-coded string in an error message costs ~50 more gas than emitting a custom error. Additionally, hard-coded strings increase the gas required to deploy the contract.

Code Location

GToken.sol

• Line 182: require(_contractAddresses.asset != address(0)...
• Line 231: require(_msgSender() == manager, "ONLY_MANAGER");
• Line 236: require(shareToAssetsPrice > 0, "PRICE_0");
• Line 237: require(assetsOrShares > 0, "VALUE_0");
• Line 242: require(maxDiscountP > 0, "NO_ACTIVE_DISCOUNT");
• Line 243: require(lockDuration >= MIN_LOCK_DURATION, "BELOW_MIN_LOCK_DURATION");
• Line 244: require(lockDuration <= MAX_LOCK_DURATION, "ABOVE_MAX_LOCK_DURATION");
• Line 250: require(newOwner != address(0), "Ownable: new owner is the zero address");
• Line 251: require(newOwner != manager && newOwner != admin, "WRONG_VALUE");
• Line 256: require(newValue != address(0), "ADDRESS_0");
• Line 257: require(newValue != owner() && newValue != admin, "WRONG_VALUE");
• Line 263: require(newValue != address(0), "ADDRESS_0");
• Line 264: require(newValue != owner() && newValue != manager, "WRONG_VALUE");
• Line 270: require(newValue != address(0), "ADDRESS_0");
• Line 276: require(newValue.addr != address(0), "ADDRESS_0");
• Line 277: require(newValue.signature.length > 0, "BYTES_0");
• Line 283: require(newValue != address(0), "ADDRESS_0");
• Line 295: require(newValue >= MIN_DAILY_ACC_PNL_DELTA, "BELOW_MIN");
• Line 301: require(newValue[1] > newValue[0], "WRONG_VALUES");
• Line 307: require(newValue <= MAX_SUPPLY_INCREASE_DAILY_P, "ABOVE_MAX");
• Line 313: require(newValue <= MAX_LOSSES_BURN_P, "ABOVE_MAX");
• Line 319: require(newValue <= MAX_GNS_SUPPLY_MINT_DAILY_P, "ABOVE_MAX");
• Line 325: require(newValue <= MAX_DISCOUNT_P, "ABOVE_MAX_DISCOUNT");
• Line 331: require(newValue >= 100 * PRECISION, "BELOW_MIN");
• Line 348: require(success == true, "GNS_PRICE_CALL_FAILED");
• Line 351: require(price > 0, "GNS_TOKEN_PRICE_0");
• Line 433: require(totalSharesBeingWithdrawn(sender) <= balanceOf(sender) - amount, "PENDING_WITHDRAWAL");
• Line 443: require(totalSharesBeingWithdrawn(from) <= balanceOf(from) - amount, "PENDING_WITHDRAWAL");
• Line 499: require(assets <= maxDeposit(receiver), "ERC4626: deposit more than max");
• Line 512: require(shares <= maxMint(receiver), "ERC4626: mint more than max");
• Line 526: require(assets <= maxWithdraw(owner), "ERC4626: withdraw more than max");
• Line 542: require(shares <= maxRedeem(owner), "ERC4626: redeem more than max");
• Line 578: require(openTradesPnlFeed.nextEpochValuesRequestCount() == 0, "END_OF_EPOCH");
• Line 582: require(sender == owner || allowance > 0 && allowance >= shares, "NOT_ALLOWED");
• Line 584: require(totalSharesBeingWithdrawn(owner) + shares <= balanceOf(owner), "MORE_THAN_BALANCE");
• Line 593: require(shares <= withdrawRequests[owner][unlockEpoch], "MORE_THAN_WITHDRAW_AMOUNT");
• Line 597: require(sender == owner || allowance > 0 && allowance >= shares, "NOT_ALLOWED");
• Line 614: require(simulatedAssets <= maxDeposit(receiver), "DEPOSIT_MORE_THAN_MAX");
• Line 630: require(shares <= maxMint(receiver), "MINT_MORE_THAN_MAX");
• Line 651: require(assets > assetsDeposited, "NO_DISCOUNT");
• Line 683: require(owner == sender...
• Line 686: require(block.timestamp >= d.atTimestamp + d.lockDuration, "NOT_UNLOCKED");
• Line 695: require(accPnlPerToken <= int(PRECISION), "NOT_ENOUGH_ASSETS");
• Line 725: require(sender == pnlHandler, "ONLY_TRADING_PNL_HANDLER");
• Line 734: require(accPnlPerToken <= int(PRECISION), "NOT_ENOUGH_ASSETS");
• Line 738: require(dailyAccPnlDelta <= int(maxDailyAccPnlDelta), "MAX_DAILY_PNL");
• Line 780: require(assets <= assetsToDeplete, "AMOUNT_TOO_BIG");
• Line 801: require(accPnlPerTokenUsed > 0, "NOT_UNDER_COLLATERALIZED");
• Line 804: require(assets <= uint(accPnlPerTokenUsed) * supply / PRECISION, "AMOUNT_TOO_BIG");
• Line 814: require(dailyMintedGns <= maxGnsSupplyMintDailyP...
• Line 839: require(sender == address(openTradesPnlFeed), "ONLY_PNL_FEED");

GTokenOpenPnlFeed.sol

• Line 104: require(_linkToken != address(0)
• Line 122: require(msg.sender == IOwnable(address(gToken)).owner(), "ONLY_OWNER");
• Line 127: require(msg.sender == gToken.manager(), "ONLY_MANAGER");
• Line 132: require(msg.sender == gToken.admin(), "ONLY_ADMIN");
• Line 138: require(newValue >= MIN_REQUESTS_START, "BELOW_MIN");
• Line 139: require(newValue <= MAX_REQUESTS_START, "ABOVE_MAX");
• Line 145: require(newValue >= MIN_REQUESTS_EVERY, "BELOW_MIN");
• Line 146: require(newValue <= MAX_REQUESTS_EVERY, "ABOVE_MAX");
• Line 152: require(newValue >= MIN_REQUESTS_COUNT, "BELOW_MIN");
• Line 153: require(newValue <= MAX_REQUESTS_COUNT, "ABOVE_MAX");
• Line 169: require(newValue >= MIN_ANSWERS, "BELOW_MIN");
• Line 170: require(newValue % 2 == 1, "EVEN");
• Line 171: require(newValue <= oracles.length / 2, "ABOVE_MAX");
• Line 177: require(_index < oracles.length, "INDEX_TOO_BIG");
• Line 178: require(newValue != address(0), "VALUE_0");
• Line 184: require(newValues.length >= minAnswers * 2, "ARRAY_TOO_SMALL");
• Line 190: require(newValue != bytes32(0), "VALUE_0");
• Line 198: require(reqToResetCount > 0, "NO_REQUEST_TO_RESET");
• Line 218: require(block.timestamp - gToken.currentEpochStart()

TWAPPriceGetter.sol

• Line 34: require(address(_uniV3Pool) != address(0)...
• Line 49: require(address(_uniV3Pool) != address(0), "WRONG_VALUE");
• Line 56: require(_twapInterval >= MIN_TWAP_PERIOD && _twapInterval <= MAX_TWAP_PERIOD, "WRONG_VALUE");

GNSTokenBridge.sol

• Line 104: require(msg.value != 0, "!fee");

NFTMintingBridge.sol

• Line 86: require(msg.value != 0, "!fee");

ACKNOWLEDGED: The Gains Trade team acknowledged this finding.

It was identified that the contracts are missing or have incomplete code documentation, which affects the understandability, auditability, and usability of the code.

Solidity contracts can use a special form of comments to provide rich documentation for functions, return variables, and more. This special form is named the Ethereum Natural Language Specification Format (NatSpec*).

ACKNOWLEDGED: The Gains Trade team acknowledged this finding.