`baseFundingRate` can be set greater than `maxBaseFundingRate` when `addAsset` is called

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/PairsContract.sol#L48-L65

Vulnerability details

Impact

When the function addAsset from the PairsContract contract is called by the owner it adds a new allowed asset, but when saving asset details into storage the function doesn't check that the value of _baseFundingRate set is less than maxBaseFundingRate.

So the owner can by accident (or intentionally) set a value bigger than the maximum, this will affect all the positions created in the Trading contract as the _updateFunding function uses the value of pairsContract.idToAsset(_asset).baseFundingRate which has the same value as the _baseFundingRate set previously.

All the functions below call the _updateFunding function and will be affected by this issue :

• initiateMarketOrder
• addToPosition
• executeLimitOrder
• liquidatePosition
• _closePosition

Proof of Concept

The issue occurs in the addAsset function :

File: contracts/PairsContract.sol Line 48-65

function addAsset(uint256 _asset, string memory _name, address _chainlinkFeed, uint256 _minLeverage, uint256 _maxLeverage, uint256 _feeMultiplier, uint256 _baseFundingRate) external onlyOwner {
    bytes memory _assetName  = bytes(_idToAsset[_asset].name);
    require(_assetName.length == 0, "Already exists");
    require(bytes(_name).length > 0, "No name");
    require(_maxLeverage >= _minLeverage && _minLeverage > 0, "Wrong leverage values");

    allowedAsset[_asset] = true;
    _idToAsset[_asset].name = _name;

    _idToAsset[_asset].chainlinkFeed = _chainlinkFeed;

    _idToAsset[_asset].minLeverage = _minLeverage;
    _idToAsset[_asset].maxLeverage = _maxLeverage;
    _idToAsset[_asset].feeMultiplier = _feeMultiplier;
    /* @audit
       There is no check on the _baseFundingRate
    */
    _idToAsset[_asset].baseFundingRate = _baseFundingRate;

    emit AssetAdded(_asset, _name);
}

As you can see from the code above there is no check on the value of _baseFundingRate and it's set directly.

The _updateFunding function will fetchs the value of baseFundingRate from the PairsContract contract and sent it to the Postion contract :

File: contracts/Trading.sol Line L817-826

function _updateFunding(uint256 _asset, address _tigAsset) internal {
    position.updateFunding(
        _asset,
        _tigAsset,
        pairsContract.idToOi(_asset, _tigAsset).longOi,
        pairsContract.idToOi(_asset, _tigAsset).shortOi,
        pairsContract.idToAsset(_asset).baseFundingRate,
        vaultFundingPercent
    );
}

And in the Postion contract the value of baseFundingRate is used to update the variable fundingDeltaPerSec of the given asset :

File: contracts/Position.sol Line 120

fundingDeltaPerSec[_asset][_tigAsset] = (_oiDelta*int256(_baseFundingRate)/int256(DIVISION_CONSTANT))/31536000;

So because the _updateFunding function is called in the following functions : initiateMarketOrder, addToPosition, executeLimitOrder, liquidatePosition, _closePosition this error will affect almost all the positions and the trading protocol in general.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue the function addAsset must contain a check on the value of _baseFundingRate, the addAsset function should be modified as follow :

function addAsset(uint256 _asset, string memory _name, address _chainlinkFeed, uint256 _minLeverage, uint256 _maxLeverage, uint256 _feeMultiplier, uint256 _baseFundingRate) external onlyOwner {
    bytes memory _assetName  = bytes(_idToAsset[_asset].name);
    require(_assetName.length == 0, "Already exists");
    require(bytes(_name).length > 0, "No name");
    require(_maxLeverage >= _minLeverage && _minLeverage > 0, "Wrong leverage values");
    /* @audit
       Add a check for : _baseFundingRate <= maxBaseFundingRate 
    */
    require(_baseFundingRate <= maxBaseFundingRate, "baseFundingRate too high");

    allowedAsset[_asset] = true;
    _idToAsset[_asset].name = _name;

    _idToAsset[_asset].chainlinkFeed = _chainlinkFeed;

    _idToAsset[_asset].minLeverage = _minLeverage;
    _idToAsset[_asset].maxLeverage = _maxLeverage;
    _idToAsset[_asset].feeMultiplier = _feeMultiplier;
    _idToAsset[_asset].baseFundingRate = _baseFundingRate;

    emit AssetAdded(_asset, _name);
}

[c4-judge]

GalloDaSballo marked the issue as primary issue

[GalloDaSballo]

Most in depth

[c4-sponsor]

TriHaz marked the issue as sponsor confirmed

[GalloDaSballo]

The warden has shown that a setter lacks a check and that can cause issues, ultimately because of the context of it being a admin function, I think QA - Low is more appropriate

[GalloDaSballo]

L

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-12-tigris-findings/issues/543