Tigris Trade contest - Aymen0909's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L255-L305

Vulnerability details

Impact

In the function addToPosition from the Trading contract the amount of open fees are handled using the _handleOpenFees function but when calling the _handleDeposit function the wrong margin is passed, in fact the _handleDeposit function gets _addMargin - _fee instead of _addMargin

So this mean that the open fees are calculated and handled but when depositing there value will not be transferred from the trader and will not be deposited in the stableVault.

Proof of Concept

The issue occurs in the addToPosition function :

File: contracts/Trading.sol Line 255-305

function addToPosition(
    uint _id,
    uint _addMargin,
    PriceData calldata _priceData,
    bytes calldata _signature,
    address _stableVault,
    address _marginAsset,
    ERC20PermitData calldata _permitData,
    address _trader
)
    external
{
    ...
    
    /* @audit
       fee are calculated and handled with _handleOpenFees
    */
    uint _fee = _handleOpenFees(_trade.asset, _addMargin*_trade.leverage/1e18, _trader, _trade.tigAsset, false);
    
    /* @audit
       But only (_addMargin - _fee) amount is deposited
    */
    _handleDeposit(
        _trade.tigAsset,
        _marginAsset,
        _addMargin - _fee,
        _stableVault,
        _permitData,
        _trader
    );
    ...
}

As you can see from the code above the _handleDeposit function receive _addMargin - _fee as new margin, this value is used to calculate the transferred amount from the trader :

File: contracts/Trading.sol Line 565-576

function _handleDeposit(address _tigAsset, address _marginAsset, uint256 _margin, address _stableVault, ERC20PermitData calldata _permitData, address _trader) internal {
    IStable tigAsset = IStable(_tigAsset);
    if (_tigAsset != _marginAsset) {
        if (_permitData.usePermit) {
            ERC20Permit(_marginAsset).permit(_trader, address(this), _permitData.amount, _permitData.deadline, _permitData.v, _permitData.r, _permitData.s);
        }
        uint256 _balBefore = tigAsset.balanceOf(address(this));
        uint _marginDecMultiplier = 10**(18-ExtendedIERC20(_marginAsset).decimals());
        
        /* @audit
           _margin == (_addMargin - _fee) is used to calculate transferred amount from trader 
           instead of the whole _addMargin value
        */
    
        IERC20(_marginAsset).transferFrom(_trader, address(this), _margin/_marginDecMultiplier);
        IERC20(_marginAsset).approve(_stableVault, type(uint).max);
        IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);
        if (tigAsset.balanceOf(address(this)) != _balBefore + _margin) revert BadDeposit();
        tigAsset.burnFrom(address(this), tigAsset.balanceOf(address(this)));
    } else {
        tigAsset.burnFrom(_trader, _margin);
    }        
}

So because of this error the open fees amount will not be transferred from the trader and will not be deposited in the StableVault.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue correct the margin passed to the function _handleDeposit, the addToPosition function should be modified as follow :

function addToPosition(
    uint _id,
    uint _addMargin,
    PriceData calldata _priceData,
    bytes calldata _signature,
    address _stableVault,
    address _marginAsset,
    ERC20PermitData calldata _permitData,
    address _trader
)
    external
{
    ...
    
    /* @audit
       fee are calculated and handled with _handleOpenFees
    */
    uint _fee = _handleOpenFees(_trade.asset, _addMargin*_trade.leverage/1e18, _trader, _trade.tigAsset, false);
    
    /* @audit
       pass correct _addMargin amount to _handleDeposit
    */
    _handleDeposit(
        _trade.tigAsset,
        _marginAsset,
        _addMargin,
        _stableVault,
        _permitData,
        _trader
    );
    ...
}

QA Report

Summary

Findings

1- owner can allow assets that are not yet added in GovNFT :

The setAllowedAsset function inside the GovNFT contract give the owner the ability to allow an asset meaning to put _allowedAsset[_asset] = true without the asset being already added to the assets array.

Risk : Low

Proof of Concept

Issue occurs in the instance below :

File: contracts/GovNFT.sol Line 307-309

/** @audit
    owner can allow asset even if they are not added yet
*/
function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
    _allowedAsset[_asset] = _bool;
}

As you can see the owner can allow any asset without having to add it with the The addAsset function, so the asset will be allowed but won't be in the asset array, this can cause many problems if functions from other contracts access directely the allowedAsset view function which will return true even if the asset is not in the asset array

Mitigation

To avoid this issue i recommend to add check statement for verifying that the asset is already added before allowing it, the function should be modified as follow :

function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
    /** @audit
        add aasset added check
    */
    require(assets[assetsIndex[_asset]] == _asset, "Not added");
    _allowedAsset[_asset] = _bool;
}

2- Risk of array length mismatch in initRefs function :

The initRefs function inside the Referrals contract is used to set an array of referral and referred, but the function doesn't check if the length of the arrays are the smae meaning that : _ownedCodes have the same length as _codeOwners and _referredA have the same length as _referredTo, so if the owner by accident sends arrays with different numbers of elements (length) the initRefs function will still set those value but in a wrong way and the owner will not be acknowledged.

Let's say for example the owner put by accident the _referredA.length == 10 and _referredTo.length == 9, the function will still store the values but when it arrives at _referred[9] it will set it as the default bytes32 value because _referredTo[9] doesn't exists.

Risk : Low

Proof of Concept

Issue occurs in the instance below :

File: contracts/Referrals.sol Line 60-76

function initRefs(
    address[] memory _codeOwners,
    bytes32[] memory _ownedCodes,
    address[] memory _referredA,
    bytes32[] memory _referredTo
) external onlyOwner {
    require(!isInit);
    /** @audit
        missing checks for :
        _codeOwners.length == _ownedCodes.length and _referredA.length == _referredTo.length
    */
    isInit = true;
    uint _codeOwnersL = _codeOwners.length;
    uint _referredAL = _referredA.length;
    for (uint i=0; i<_codeOwnersL; i++) {
        _referral[_ownedCodes[i]] = _codeOwners[i];
    }
    for (uint i=0; i<_referredAL; i++) {
        _referred[_referredA[i]] = _referredTo[i];
    }
}

Mitigation

To avoid this issue i recommend to add check statement of the arrays lengths, the function should be modified as follow :

function initRefs(
    address[] memory _codeOwners,
    bytes32[] memory _ownedCodes,
    address[] memory _referredA,
    bytes32[] memory _referredTo
) external onlyOwner {
    require(!isInit);
    /** @audit
        add array lengths checks
    */
    require(_codeOwners.length == _ownedCodes.length, "length mismatch");
    require(_referredA.length == _referredTo.length, "length mismatch");
    isInit = true;
    uint _codeOwnersL = _codeOwners.length;
    uint _referredAL = _referredA.length;
    for (uint i=0; i<_codeOwnersL; i++) {
        _referral[_ownedCodes[i]] = _codeOwners[i];
    }
    for (uint i=0; i<_referredAL; i++) {
        _referred[_referredA[i]] = _referredTo[i];
    }
}

3- maxbaseFundingRate is not bounded :

The maxbaseFundingRate variable is used to prevent the owner from setting a large value for baseFundingRate variable when calling the setAssetBaseFundingRate function, this illustrated with this check :

File: contracts/PairsContract.sol Line 95

require(_baseFundingRate <= maxBaseFundingRate, "baseFundingRate too high");

But the value of maxbaseFundingRate itself is variable and doesn't have a max value, in fact the owner can call the setMaxBaseFundingRate function and sets maxBaseFundingRate = type(uint256).max, which will also cause the baseFundingRate variable to not have a max value.

So the way this system is designed is not really working unless the owner is a governace DAO (must go through voting to set a new maxbaseFundingRate)

Risk : Low

Proof of Concept

Issue occurs in the instance below :

File: contracts/PairsContract.sol Line 125-127

/** @audit
    owner can set maxBaseFundingRate to any value
*/
function setMaxBaseFundingRate(uint256 _maxBaseFundingRate) external onlyOwner {
    maxBaseFundingRate = _maxBaseFundingRate;
}

As you can see the owner can set any value for maxbaseFundingRate variables even type(uint256).max

Mitigation

To avoid this issue i recommend to add a constant maximum value MAX_RATE for maxbaseFundingRate which everyone can see this still allows the owner to change the maxbaseFundingRate value but it will now be bounded, the setMaxBaseFundingRate function should be modified as follow :

/** @audit
    add a max value for maxBaseFundingRate
*/
uint256 private constant MAX_RATE = 1e10;
function setMaxBaseFundingRate(uint256 _maxBaseFundingRate) external onlyOwner {
    require(_maxBaseFundingRate <= MAX_RATE, "too high");
    maxBaseFundingRate = _maxBaseFundingRate;
}

4- bond owner is set to address(0) when createLock is called :

When the createLock is called it should creates a new bond for the given owner address but inside the Bond struct the owner address is set to address(0) instead of the real owner address _owner, in the current implementation of the BondNFT contract this hasn't a big impact as the method ownerOf(bond.id) is used every time to get the owner but for futur developoment it must be taken into consideration.

Risk : Low

Proof of Concept

The issue occurs in the createLock function below :

File: contracts/BondNFT.sol Line 57-86

function createLock(
    address _asset,
    uint _amount,
    uint _period,
    address _owner
) external onlyManager() returns(uint id) {
    require(allowedAsset[_asset], "!Asset");
    unchecked {
        uint shares = _amount * _period / 365;
        uint expireEpoch = epoch[_asset] + _period;
        id = ++totalBonds;
        totalShares[_asset] += shares;
        Bond memory _bond = Bond(
            id,             // id
            /** @audit
                owner value is set to address(0) instead of _owner
            */
            address(0),     // owner
            _asset,         // tigAsset token
            _amount,        // tigAsset amount
            epoch[_asset],  // mint epoch
            block.timestamp,// mint timestamp
            expireEpoch,    // expire epoch
            0,              // pending
            shares,         // linearly scaling share of rewards
            _period,        // lock period
            false           // is expired boolean
        );
        _idToBond[id] = _bond;
        _mint(_owner, _bond);
    }
    emit Lock(_asset, _amount, _period, _owner, id);
}

Mitigation

To remove this issue there two options :

• Set the correct owner in the Bond struct bby replacing address(0) by _owner.

• Remove completly the owner field from the Bond struct as the owner address can be obtained from the minted NFT owner by using method ownerOf.

5- Error in withdraw function Natspec :

In the withdraw function inside the StableVault contract the Natspec of the function says that the parameter _amount refers to the amount of _token received, but when you read the function you notice that it refers in reality to the amount of tigAsset swapped for _token.

Risk : Low

Proof of Concept

Issue occurs in the instance below :

File: contracts/StableVault.sol Line 60-72

/**
* @notice swap tigAsset to _token
* @param _token address of the token to receive
* @param _amount amount of _token
*/
function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {
    /* @audit
       _amount refers to tigAsset not to _token
    */
    IERC20Mintable(stable).burnFrom(_msgSender(), _amount);
    _output = _amount/10**(18-IERC20Mintable(_token).decimals());
    IERC20(_token).transfer(
        _msgSender(),
        _output
    );
}

Mitigation

If this is a simple typo it should be corrected as it can be misleading for both devs and users, but if the _amount argument really mented to be the amount of _token that should be withdrawn then the function should be reviewed to correct the math and transfer the right amount.

6- Using deprecated Chainlink function latestAnswer could result in wrong prices :

In the TradingLibrary library the function verifyPrice is used to verify the trade price and if the variables _chainlinkEnabled is set to true the function will use the Chainlink price feed to get the latest price for the given asset, but the function verifyPrice uses a deprecated Chainlink function latestAnswer to get the price of the token as it's mentionned here, this function does not throw an error if no answer has been reached but returns 0 and it can give a stale price for a given token which would impact on the trading logic.

Risk : Low

Proof of Concept

Issue occurs in the verifyPrice function:

File: contracts/utils/TradingLibrary.sol Line 91-122

function verifyPrice(
    uint256 _validSignatureTimer,
    uint256 _asset,
    bool _chainlinkEnabled,
    address _chainlinkFeed,
    PriceData calldata _priceData,
    bytes calldata _signature,
    mapping(address => bool) storage _isNode
)
    external view
{
    ...

    if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
        int256 assetChainlinkPriceInt = IPrice(_chainlinkFeed).latestAnswer();
        if (assetChainlinkPriceInt != 0) {
            uint256 assetChainlinkPrice = uint256(assetChainlinkPriceInt) * 10**(18 - IPrice(_chainlinkFeed).decimals());
            require(
                _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
                _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
            );
        }
    }
}

Even though the issue of latestAnswer not reverting on non-response is handled with the check if (assetChainlinkPriceInt != 0), the problem of stale price still remaigns.

Mitigation

To avoid the risk of getting stale price from Chainlink oracle i recommend to use the latestRoundData function with it's required checks.

The function verifyPrice should be modified as follow :

function verifyPrice(
    uint256 _validSignatureTimer,
    uint256 _asset,
    bool _chainlinkEnabled,
    address _chainlinkFeed,
    PriceData calldata _priceData,
    bytes calldata _signature,
    mapping(address => bool) storage _isNode
)
    external view
{
    ...

    if (_chainlinkEnabled && _chainlinkFeed != address(0)) {
        (
            uint80 roundID,
            uint256 assetPrice,
            ,
            uint256 timestamp,
            uint80 answeredInRound
        ) = IPrice(_chainlinkFeed).latestRoundData();

        require(assetPrice > 0, "Invalid feed price");
        require(answeredInRound >= roundID, "Stale price");
        require(timestamp != 0, "Round not complete");

        uint256 assetChainlinkPrice = uint256(assetPrice) * 10**(18 - IPrice(_chainlinkFeed).decimals());
        require(
            _priceData.price < assetChainlinkPrice+assetChainlinkPrice*2/100 &&
            _priceData.price > assetChainlinkPrice-assetChainlinkPrice*2/100, "!chainlinkPrice"
        );
        
    }
}

7- Use call() instead of transfer() :

When transferring ETH, call() should be used instead of transfer().

The transfer() function only allows the recipient to use 2300 gas. If the recipient uses more than that, transfers will fail. In the future gas costs might change increasing the likelihood of that happening.

Keep in mind that call() introduces the risk of reentrancy, but as the code follows the CEI pattern it should be fine.

Risk : Low

Proof of Concept

Instances include:

File: contracts/Trading.sol Line 588

payable(_proxy).transfer(msg.value);

Mitigation

Replace transfer() calls with call(), keep in mind to check whether the call was successful by validating the return value.

8- Immutable state variables lack zero address checks :

Constructors should check the values written in an immutable state variables(address) is not the address(0)

Risk : Low

Proof of Concept

Instances include:

File: contracts/Lock.sol Line 25-26

bondNFT = IBondNFT(_bondNFTAddress);
govNFT = IGovNFT(_govNFT);

File: contracts/StableVault.sol Line 36

stable = _stable;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

9- Duplicated require() checks should be refactored to a modifier :

require() checks statement used multiple times inside a contract should be refactored to a modifier for better readability.

Risk : NON CRITICAL

Proof of Concept

There are 6 instances of this issue in PairsContract.sol:

File: contracts/PairsContract.sol

35      require(_name.length > 0, "!Asset");
75      require(_name.length > 0, "!Asset");
94      require(_name.length > 0, "!Asset");
106     require(_name.length > 0, "!Asset");
117     require(_name.length > 0, "!Asset");
141     require(_name.length > 0, "!Asset");

Those checks should be replaced by a Exists modifier as follow :

modifier Exists(uint256 _asset){
    bytes memory _name  = bytes(_idToAsset[_asset].name);
    require(_name.length > 0, "!Asset");
    _;
}

10- contracts/libraries imported but not used :

The contracts, interfaces or libraries imported in a contract and are never used should be removed.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/Lock.sol Line 4

import "hardhat/console.sol";

File: contracts/StableVault.sol Line 5

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

Mitigation

Remove the imported contracts/libraries if they are not intended to be used.

11- Use solidity time-based units :

Solidity contains time-based units (seconds, minutes, hours, days and weeks) which should be used when declaring time based variables/constants instead of using literal numbers, this is done for better readability and to avoid mistakes.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/BondNFT.sol Line 10

uint constant private DAY = 24 * 60 * 60;

Mitigation

Use time units when declaring time-based variables.

12- Use scientific notation :

When using multiples of 10 you shouldn't use decimal literals or exponentiation (e.g. 1000000, 10**18) but use scientific notation for better readability.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: contracts/GovNFT.sol Line 16

uint256 private constant MAX = 10000;

File: contracts/GovNFT.sol Line 17

uint256 public gas = 150000;

File: contracts/GovNFT.sol Line 66

require(tokenId <= 10000, "BadID");

File: contracts/TradingExtension.sol Line 26

uint public maxGasPrice = 1000000000000;

Mitigation

Use scientific notation for the instances aforementioned.

Gas Optimizations

Summary

Findings

1- State variables only set in the constructor should be declared immutable :

The state variables only set in the constructor should be declared immutable, this avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There are 7 instances of this issue:

File: contracts/Trading.sol Line 121-123

IPairsContract private pairsContract;
IPosition private position;
IGovNFT private gov;

File: contracts/TradingExtension.sol Line 13

address public trading;

File: contracts/TradingExtension.sol Line 22-24

IPairsContract private pairsContract;
IReferrals private referrals;
IPosition private position;

2- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.

There is 1 instance of this issue:

File: contracts/BondNFT.sol Line 12-24

struct Bond {
    uint256 id;
    address owner;
    address asset;
    uint256 amount;
    uint256 mintEpoch;
    uint256 mintTime;
    uint256 expireEpoch;
    uint256 pending;
    uint256 shares;
    uint256 period;
    bool expired;
}

As the mintEpoch, mintTime, expireEpoch and period parameters values can't really overflow 2^64 their values should be packed together and the struct should be modified as follow to save gas :

struct Bond {
    uint256 id;
    address owner;
    address asset;
    uint256 amount;    
    uint256 pending;
    uint256 shares;
    uint64 mintEpoch;
    uint64 mintTime;
    uint64 expireEpoch;
    uint64 period;
    bool expired;
}

3- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 10 instances of this issue :

File: contracts/BondNFT.sol

26      mapping(address => uint256) public epoch;
32      mapping(address => bool) public allowedAsset;
33      mapping(address => uint256) private assetsIndex;
35      mapping(address => mapping(uint256 => uint256)) private accRewardsPerShare; // tigAsset => epoch => accRewardsPerShare
37      mapping(address => uint256) public totalShares;

These mappings could be refactored into the following struct and mapping for example :

struct Asset {
    uint256 epoch;
    uint256 assetsIndex;
    uint256 totalShares;
    bool allowedAsset;
    mapping(uint256 => uint256) accRewardsPerShare;
}
    
mapping(address => Asset) private _assets;

File: contracts/GovNFT.sol

265     mapping(address => bool) private _allowedAsset;
266     mapping(address => uint) private assetsIndex;
269     mapping(address => uint256) private accRewardsPerNFT;

These mappings could be refactored into the following struct and mapping for example :

struct Asset {
    uint256 assetsIndex;
    uint256 accRewardsPerNFT;
    bool allowedAsset;
}
    
mapping(address => Asset) private _assets;

File: contracts/StableVault.sol

29      mapping(address => bool) public allowed;
30      mapping(address => uint) private tokenIndex;

These mappings could be refactored into the following struct and mapping for example :

struct Token {
    uint256 tokenIndex;
    bool allowed;
}
    
mapping(address => Token) private _tokens;

4- Remove unecessary owner field in Bond struct to save gas :

The owner address stored in the Bond struct is unecessary as each bond is minted as an NFT, so with the bond id you can easily get the owner by calling ownerOf(bond.id), and the fact that the bond.owner is set to address(0) in createLock function and never get used later as all the functions call idToBond to get bond info which uses ownerOf(_id) method.

As you can see the idToBond function uses directly ownerOf(_id) method :

File: contracts/BondNFT.sol Line 237

function idToBond(uint256 _id) public view returns (Bond memory bond) {
    bond = _idToBond[_id];
    /**
        Use of ownerOf(_id) directly 
     */
    bond.owner = ownerOf(_id);
    bond.expired = bond.expireEpoch <= epoch[bond.asset] ? true : false;
    unchecked {
        uint _accRewardsPerShare = accRewardsPerShare[bond.asset][bond.expired ? bond.expireEpoch-1 : epoch[bond.asset]];
        bond.pending = bond.shares * _accRewardsPerShare / 1e18 - bondPaid[_id][bond.asset];
    }
}

So i recommend to remove the owner field from Bond struct to save gas when calling for example createLock function (because there is less value to store).

5- Misorganized if statement in claim function cost more gas :

The issue occurs in the code below :

File: contracts/BondNFT.sol Line 168-187

function claim(
    uint _id,
    address _claimer
) public onlyManager() returns(uint amount, address tigAsset) {
    Bond memory bond = idToBond(_id);
    require(_claimer == bond.owner, "!owner");
    amount = bond.pending;
    tigAsset = bond.asset;
    unchecked {
        if (bond.expired) {
            /** @audit
                Unecessary calculation of _pendingDelta if totalShares[bond.asset] == 0
            */
            uint _pendingDelta = (bond.shares * accRewardsPerShare[bond.asset][epoch[bond.asset]] / 1e18 - bondPaid[_id][bond.asset]) - (bond.shares * accRewardsPerShare[bond.asset][bond.expireEpoch-1] / 1e18 - bondPaid[_id][bond.asset]);
            if (totalShares[bond.asset] > 0) {
                accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
            }
        }
        bondPaid[_id][bond.asset] += amount;
    }
    IERC20(tigAsset).transfer(manager, amount);
    emit ClaimFees(tigAsset, amount, _claimer, _id);
}

As you can see from the code above if we have bond.expired == true then the function calculate the _pendingDelta but this calculation could be unecessary if we also have totalShares[bond.asset] == 0, and because the _pendingDelta calculation requires multiple reads from storage it will cost additional gas for nothing.

So i recommend to modify the if checks to avoid this issue, the new code should look like this :

function claim(
    uint _id,
    address _claimer
) public onlyManager() returns(uint amount, address tigAsset) {
    ...
    unchecked {
        /** @audit
            Check both bond.expired and totalShares[bond.asset]>0 at the same time
            This avoids unecessary calculation of _pendingDelta if totalShares[bond.asset] == 0
        */
        if (bond.expired && totalShares[bond.asset] > 0) {
            uint _pendingDelta = (bond.shares * accRewardsPerShare[bond.asset][epoch[bond.asset]] / 1e18 - bondPaid[_id][bond.asset]) - (bond.shares * accRewardsPerShare[bond.asset][bond.expireEpoch-1] / 1e18 - bondPaid[_id][bond.asset]);
            accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
        }
        bondPaid[_id][bond.asset] += amount;
    }
    ...
}

6- assetsLength() should not be called at each loop iteration :

The assetsLength() function reads directly from storage to get assets.length, so using this function to get the length in a for loop means that in every iteration there will be a read from storage which will cost a lot of gas.

There are 4 instances of this issue :

File: contracts/GovNFT.sol

53      for (uint i=0; i<assetsLength(); i++)
67      for (uint i=0; i<assetsLength(); i++)
78      for (uint i=0; i<assetsLength(); i++)
95      for (uint i=0; i<assetsLength(); i++)

I recommend to call the assetsLength() function before the for loop and store its return values in a memory variable this will save a lot of gas, for example :

uint256 len = assetsLength();

for (uint i=0; i<len; i++){
    ...
}

7- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 2 instances of this issue :

File: contracts/GovNFT.sol Line 80-81

userDebt[owner][assets[i]] -= userPaid[owner][assets[i]]/balanceOf(owner);
userPaid[owner][assets[i]] -= userPaid[owner][assets[i]]/balanceOf(owner);     

The above operations should be modified as follow :

uint256 _amount = userPaid[owner][assets[i]]/balanceOf(owner);
userDebt[owner][assets[i]] -= _amount;
userPaid[owner][assets[i]] -= _amount;     

File: contracts/GovNFT.sol Line 97-98

userDebt[from][assets[i]] -= userPaid[from][assets[i]]/balanceOf(from);
userPaid[from][assets[i]] -= userPaid[from][assets[i]]/balanceOf(from);     

The above operations should be modified as follow :

uint256 _amount = userPaid[from][assets[i]]/balanceOf(from);
userDebt[from][assets[i]] -= _amount;
userPaid[from][assets[i]] -= _amount;      

8- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There is 1 instance of this issue:

File: contracts/GovNFT.sol Line 52

counter += 1;  

The above operation cannot overflow due to the check require(counter <= MAX, "Exceeds supply"); It should be marked as unchecked.

9- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 30 instances of this issue:

File: contracts/BondNFT.sol

68      totalShares[_asset] += shares;
115     totalShares[bond.asset] += shares-bond.shares;
117     _bond.amount += _amount;
119     _bond.period += _period;
150     totalShares[bond.asset] -= bond.shares;
180     accRewardsPerShare[bond.asset][epoch[bond.asset]] += _pendingDelta*1e18/totalShares[bond.asset];
183     bondPaid[_id][bond.asset] += amount;
221     epoch[_tigAsset] += 1;
225     accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];
333     userDebt[from][bond.asset] += bond.pending;
334     bondPaid[_id][bond.asset] += bond.pending;

File: contracts/GovNFT.sol

54      userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
68      userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
79      userDebt[owner][assets[i]] += accRewardsPerNFT[assets[i]];       
80      userDebt[owner][assets[i]] -= userPaid[owner][assets[i]]/balanceOf(owner);
81      userPaid[owner][assets[i]] -= userPaid[owner][assets[i]]/balanceOf(owner);
96      userDebt[from][assets[i]] += accRewardsPerNFT[assets[i]];
97      userDebt[from][assets[i]] -= userPaid[from][assets[i]]/balanceOf(from);
98      userPaid[from][assets[i]] -= userPaid[from][assets[i]]/balanceOf(from);
99      userPaid[to][assets[i]] += accRewardsPerNFT[assets[i]];
278     userPaid[_msgsender][_tigAsset] += amount;
290     accRewardsPerNFT[_tigAsset] += _amount/totalSupply();

File: contracts/Lock.sol

73      totalLocked[_asset] += _amount;
103     totalLocked[asset] -= lockAmount;

File: contracts/PairsContract.sol

156     _idToOi[_asset][_tigAsset].longOi += _amount;
160     _idToOi[_asset][_tigAsset].longOi -= _amount;
176     _idToOi[_asset][_tigAsset].shortOi += _amount;
180     _idToOi[_asset][_tigAsset].shortOi -= _amount;

File: contracts/Position.sol

231     _trades[_id].accInterest -= _trades[_id].accInterest*int256(_percent)/int256(DIVISION_CONSTANT);
232     _trades[_id].margin -= _trades[_id].margin*_percent/DIVISION_CONSTANT;

10- require() strings longer than 32 bytes cost extra gas :

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas.

There are 4 instances of this issue :

File: contracts/GovNFT.sol

153     require(
            msg.value >= messageFee,
            "Must send enough value to cover messageFee"
        );
185     require(msg.sender == address(this), "NonblockingLzApp: caller must be app");
209     require(payloadHash != bytes32(0), "NonblockingLzApp: no stored message");
210     require(keccak256(_payload) == payloadHash, "NonblockingLzApp: invalid payload");

11- Splitting require() statements that uses && saves gas :

There are 2 instances of this issue :

File: contracts/PairsContract.sol

52      require(_maxLeverage >= _minLeverage && _minLeverage > 0, "Wrong leverage values");

File: contracts/Trading.sol

887     require(_proxy.proxy == _msgSender() && _proxy.time >= block.timestamp, "Proxy not approved");

12- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 16 instances of this issue:

File: contracts/GovNFT.sol

53      for (uint i=0; i<assetsLength(); i++)
67      for (uint i=0; i<assetsLength(); i++)
78      for (uint i=0; i<assetsLength(); i++)
95      for (uint i=0; i<assetsLength(); i++)
105     for (uint i=0; i<_amount; i++)
131     for (uint i=0; i<tokenId.length; i++)
200     for (uint i=0; i<tokenId.length; i++)
246     for (uint i=0; i<_ids.length; i++)
252     for (uint i=0; i<_ids.length; i++)
258     for (uint i=0; i<_ids.length; i++)
325     for (uint i=0; i<_ids.length; i++)

File: contracts/Lock.sol

113     for (uint i=0; i < assets.length; i++) 

File: contracts/Position.sol

296     for (uint i=0; i<_ids.length; i++)
304     for (uint i=0; i<_ids.length; i++)

File: contracts/Referrals.sol

70      for (uint i=0; i<_codeOwnersL; i++)
73      for (uint i=0; i<_referredAL; i++)