code-423n4 / 2022-12-tigris-findings

8 stars 4 forks source link

Contract Owner Possesses Too Many Privileges #648

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The owner has many privileges in the contract: setBlockDelay, setAllowedVault, setMaxWinPorcent, setLimitOrdenPriceRange, setFees, setTradingExtension

Proof of Concept

  function setBlockDelay(
    uint _blockDelay
)
    external
    onlyOwner
{
    blockDelay = _blockDelay;
}

/**
 * @dev Whitelists a stablevault contract address
 * @param _stableVault StableVault address
 * @param _bool true if allowed
 */
function setAllowedVault(
    address _stableVault,
    bool _bool
)
    external
    onlyOwner
{
    allowedVault[_stableVault] = _bool;
}

/**
 * @dev Sets max payout % compared to margin
 * @param _maxWinPercent payout %
 */
function setMaxWinPercent(
    uint _maxWinPercent
)
    external
    onlyOwner
{
    maxWinPercent = _maxWinPercent;
}

/**
 * @dev Sets executable price range for limit orders
 * @param _range price range in %
 */
function setLimitOrderPriceRange(uint _range) external onlyOwner {
    limitOrderPriceRange = _range;
}

/**
 * @dev Sets the fees for the trading protocol
 * @param _open True if open fees are being set
 * @param _daoFees Fees distributed to the DAO
 * @param _burnFees Fees which get burned
 * @param _referralFees Fees given to referrers
 * @param _botFees Fees given to bots that execute limit orders
 * @param _percent Percent of earned funding fees going to StableVault
 */
function setFees(bool _open, uint _daoFees, uint _burnFees, uint _referralFees, uint _botFees, uint _percent) external onlyOwner {
    unchecked {
        require(_daoFees >= _botFees+_referralFees*2);
        if (_open) {
            openFees.daoFees = _daoFees;
            openFees.burnFees = _burnFees;
            openFees.referralFees = _referralFees;
            openFees.botFees = _botFees;
        } else {
            closeFees.daoFees = _daoFees;
            closeFees.burnFees = _burnFees;
            closeFees.referralFees = _referralFees;
            closeFees.botFees = _botFees;                
        }
        require(_percent <= DIVISION_CONSTANT);
        vaultFundingPercent = _percent;
    }
}

/**
 * @dev Sets the extension contract address for trading
 * @param _ext extension contract address
 */
function setTradingExtension(
    address _ext
) external onlyOwner() {
    tradingExtension = ITradingExtension(_ext);
}

// ===== EVENTS =====

event PositionOpened(
    TradeInfo _tradeInfo,
    uint _orderType,
    uint _price,
    uint _id,
    address _trader,
    uint _marginAfterFees
);

event PositionClosed(
    uint _id,
    uint _closePrice,
    uint _percent,
    uint _payout,
    address _trader,
    address _executor
);

event PositionLiquidated(
    uint _id,
    address _trader,
    address _executor
);

event LimitOrderExecuted(
    uint _asset,
    bool _direction,
    uint _openPrice,
    uint _lev,
    uint _margin,
    uint _id,
    address _trader,
    address _executor
);

event UpdateTPSL(
    uint _id,
    bool _isTp,
    uint _price,
    address _trader
);

event LimitCancelled(
    uint _id,
    address _trader
);

event MarginModified(
    uint _id,
    uint _newMargin,
    uint _newLeverage,
    bool _isMarginAdded,
    address _trader
);

event AddToPosition(
    uint _id,
    uint _newMargin,
    uint _newPrice,
    address _trader
);

event FeesDistributed(
    address _tigAsset,
    uint _daoFees,
    uint _burnFees,
    uint _refFees,
    uint _botFees,
    address _referrer
);

tool used

manual

TriHaz commented 1 year ago

We are aware of the centralization risks, initially, all contracts will have a multi-sig as owner to prevent a sole owner, later on a DAO could be the owner.

c4-sponsor commented 1 year ago

TriHaz marked the issue as sponsor acknowledged

GalloDaSballo commented 1 year ago

This in conjunction with #377 covers all "basic" admin privilege findings

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #377

c4-judge commented 1 year ago

GalloDaSballo marked the issue as satisfactory