code423n4 commented 1 year ago

Lines of code

Vulnerability details

Malicious or compromised owner can pause assets forever

Summary

Malicious or compromised owner can pause assets without any kind of timelock or event emission, making much less trustable as assets can get locked in just a fast call.

Impact

Lock of funds

Vulnerability Details

Security pitfalls and best practices: Point 114 Token is not pausable: Malicious or compromised owners can trap contracts relying on pausable tokens. (See here)

Proof of Concept

Over the contract there are different instances of functions for pausing assets to be traded, claim rewards, etc.

This can cause the user to have locked rewards and funds that can't be traded. As this changes of the allowedAsset list are not notice by any event, this can lead into user getting assets that won't be able to trade.

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/PairsContract.sol#L115-L119

     /**
     * @notice pause an asset from being traded
     * @param _asset index of the asset
     * @param _isPaused paused if true
     */
    function pauseAsset(uint256 _asset, bool _isPaused) external onlyOwner {
        bytes memory _name  = bytes(_idToAsset[_asset].name);
        require(_name.length > 0, "!Asset");
        allowedAsset[_asset] = !_isPaused;//@audit no emit //@audit only owner 
    }

That affects validation of trades in TradingExtension

    /**
     * @dev validates the inputs of trades
     * @param _asset asset id
     * @param _tigAsset margin asset
     * @param _margin margin
     * @param _leverage leverage
     */
    function validateTrade(uint _asset, address _tigAsset, uint _margin, uint _leverage) external view {
        unchecked {
            IPairsContract.Asset memory asset = pairsContract.idToAsset(_asset);
            if (!allowedMargin[_tigAsset]) revert("!margin");
            if (paused) revert("paused");//@audit-ok can't trade if paused
            if (!pairsContract.allowedAsset(_asset)) revert("!allowed");//@audit can't trade if asset paused (blacklisted or no whitelisted)

That is later called in Trading.sol at initiateMarketOrder(), addToPosition() and initiateLimitOrder()

Other functions that change allowedAsset https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/GovNFT.sol#L307-L309

    function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
        _allowedAsset[_asset] = _bool;
    }//@audit-info no timelock for change of allowedAsset //@audit-info only owner //@audit-info no emit

This prevents users to claim their rewards https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/GovNFT.sol#L287-L294

    /**
    * @notice add rewards for NFT holders
    * @param _tigAsset reward token address
    * @param _amount amount to be distributed
    */
    function distribute(address _tigAsset, uint _amount) external {
        if (assets.length == 0 || assets[assetsIndex[_tigAsset]] == address(0) || totalSupply() == 0 || !_allowedAsset[_tigAsset]) return;
        try IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount) {
            accRewardsPerNFT[_tigAsset] += _amount/totalSupply();
        } catch {

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L126-L132

    /**
     * @notice Whitelist an asset
     * @param _tigAsset tigAsset token address
     * @param _isAllowed set tigAsset as allowed
     */
    function editAsset(
        address _tigAsset,
        bool _isAllowed
    ) external onlyOwner() {
        allowedAssets[_tigAsset] = _isAllowed;//@audit-info no timelock for change of allowedAsset //@audit-info only owner //@audit-info no emit
    }

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L61-L76

     * @notice Lock up tokens to create a bond
     * @param _asset tigAsset being locked
     * @param _amount tigAsset amount
     * @param _period number of days to be locked for
     */
    function lock(//@audit-ok everybody can call it
        address _asset,
        uint _amount,
        uint _period
    ) public {
        require(_period <= maxPeriod, "MAX PERIOD");
        require(_period >= minPeriod, "MIN PERIOD");
        require(allowedAssets[_asset], "!asset");

And finally not in the user side

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/BondNFT.sol#L357-L360

    function setAllowedAsset(address _asset, bool _bool) external onlyOwner {
        require(assets[assetsIndex[_asset]] == _asset, "Not added");
        allowedAsset[_asset] = _bool;//@audit-info no timelock for change of allowedAsset //@audit-info only owner //@audit-info no emit
    }

This would prevent the manager role (only set by the owner) to create bonds

    function createLock(
        address _asset,
        uint _amount,
        uint _period,
        address _owner
    ) external onlyManager() returns(uint id) {
        require(allowedAsset[_asset], "!Asset");

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/BondNFT.sol#L57-L63

Recommended Mitigation Steps

In case of wanting to keep the functionality, add an event emission in order to be able to be read off-chain and therefore notice on time.

Also add a timelock of a reasonable time in order to improve trust by the users.

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. Disagree with severity, should be med I guess?