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

8 stars 4 forks source link

Lack of reasonable boundary for parameter setting in Trading.sol #529

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L898 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L952 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L926 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/Trading.sol#L939

Vulnerability details

Impact

Lack of reasonable boundary for fee setting

Proof of Concept

the fee structured is listed in the doc

https://docs.tigris.trade/protocol/trading-and-fees#fee-structure

and the liquidation fee is listed in

https://docs.tigris.trade/protocol/trading-and-fees/liquidation

note the doc:

Liquidations are performed by bots that are paid with 0.01% of the position size. The rest remains inside the StableVault to increase its collateralization levels.

However, in the code, there is no such implementation about fee setting in the doc and no such restriction in fee setting:

the owner can set fee whatever they want.

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;
    }
}

Same issue for parameter settings which lacks of reasonable boundry is in the code below as well:

/**
 * @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;
}

and in block delay:

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

https://docs.tigris.trade/protocol/trading-and-fees#limitations

note the doc:

All trading orders need to confirmed within 10 seconds, otherwise the locked in price will expire.

But different blockchain has different confirmation time, using 10 seconds for all blockchain is too harsh (for example, ethereum has blockchain confirmation of 12 - 15 seconds).

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project add reasonable upper and lower bound for parameter setting to conform the document.

TriHaz commented 1 year ago

Duplicate of #15

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #514

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