Tigris Trade contest - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L84-L92

Vulnerability details

Impact

Lock.extendLock doesn't increase totalLocked variable. As result some users will not be able to releaseLock.

Proof of Concept

When new lock is created using Lock.lock function then totalLocked variable is increased with provided amount. But in case of extendLock function there is no totalLocked variable updating. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L84-L92

    function extendLock(
        uint _id,
        uint _amount,
        uint _period
    ) public {
        address _asset = claim(_id);
        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        bondNFT.extendLock(_id, _asset, _amount, _period, msg.sender);
    }

Later when user wants to release lock totalLocked is decreased with amount locked.

The problem here next. 1.User deposits 100 tokens. 2.User extends lock with more 100 tokens. 3.User releases lock and lockAmount is 200 and release function reverts with underflow error as totalLocked is 100 currently.

Tools Used

VsCode

Recommended Mitigation Steps

Update totalLocked variable inside extendLock function.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L168-L187

Vulnerability details

Impact

BondNFT.claim can be called several times to increase rewards amount.

Proof of Concept

BondNft.claim function is called to claim bond rewards that have accrued during the time. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L168-L187

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

If bond is already expired then function distributes the amount that is accrued in next epochs after bond has expired to bonds that are nor expired yet.

 if (bond.expired) {
                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];
                }
            }

The problem is that currently this function can be called as many times as you wish using Lock.claim function. So that means that accRewardsPerShare[bond.asset][epoch[bond.asset]] will be always increasing.

Attack example. 1.Attacker creates 2 bonds. One is for short period, another is for little bit longer period. 2.When first bond has expired attacker calls claim too many times to increase accRewardsPerShare[bond.asset][epoch[bond.asset]] value to drain all funds from BondNft. 3.After that attacker claims his another bond and recieve all funds that are in BondNft as reward.

Tools Used

VsCode

Recommended Mitigation Steps

Make sure that accRewardsPerShare updating is made only once. Such check may help if (bond.expired && bond.pending > 0).

Lines of code

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

Vulnerability details

Impact

Trading.addToPosition don't take fees from trader. Protocol loses funds.

Proof of Concept

Trading.addToPosition function allows trader to add additional margin to existing position. First it calculates fees and then receives payment from trader. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L274-L282

        uint _fee = _handleOpenFees(_trade.asset, _addMargin*_trade.leverage/1e18, _trader, _trade.tigAsset, false);
        _handleDeposit(
            _trade.tigAsset,
            _marginAsset,
            _addMargin - _fee,
            _stableVault,
            _permitData,
            _trader
        );

The problem here is that amount that should be transferred from user is passed as _addMargin - _fee, but fee should also be passed. Otherwise the operation was for free and protocol loses funds.

Tools Used

VsCode

Recommended Mitigation Steps

Provide _addMargin param as amount to receive from trader.

Lines of code

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

Vulnerability details

Impact

Because function Trading._handleDeposit every time approves _stableVault contract for max allowance it will work only once for such tokens as usdt.

Proof of Concept

Trading._handleDeposit function is used in Trading contract to get funds from trader. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L643-L659

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

The line that we need to check is IERC20(_marginAsset).approve(_stableVault, type(uint).max);. In this line Trading contract set max allowance for vault contract in marginAsset token. In case if such token is USDT or similar(that doesn't allow to provide allowance if current allowance was not 0) will be used that means that this function will succeed only one time. On all next calls it will revert.

Because there is no function that allows to set allowance for USDT token to 0 it means that it will not be possible to use USDT anymore.

I believe this is high severity issue as it will fully block ability to use USDT token with Trading contract.

Tools Used

VsCode

Recommended Mitigation Steps

Set allowance to 0 after transfer, or set allowance equal to the value you want to be transferred.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L235 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L847-L849

Vulnerability details

Impact

Approved operators of owner of Position token can't call several function in Trading.

Proof of Concept

Functions that accept Position token in Trading are checking that the caller is owner of token using _checkOwner function. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Trading.sol#L847-L849

    function _checkOwner(uint _id, address _trader) internal view {
        if (position.ownerOf(_id) != _trader) revert("2"); //NotPositionOwner   
    }

As you can see this function doesn't allow to approved operators of token's owner to pass the check. As result functions are not possible to call for them on behalf of owner. For example here there is a check that doesn't allow to call initiateCloseOrder function.

Tools Used

VsCode

Recommended Mitigation Steps

Allow operators of token's owner to call functions on behalf of owner.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/StableVault.sol#L49

Vulnerability details

Impact

StableVault doesn't support tokens that has more than 18 decimals

Proof of Concept

When user deposits to StableVault, provided value is scaled to 18 decimals using formula _amount*(10**(18-IERC20Mintable(_token).decimals()). In case if token decimals is more than 18 this will always revert with underflow error. As result StableVault can't work with tokens that has more decimals than 18.

Tools Used

VsCode

Recommended Mitigation Steps

Check that token decimals is not bigger than 18 when adding new token or add support for tokens with more decimals than 18.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L168-L187 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L150

Vulnerability details

Impact

When BondNft.claim is called before BondNft.release for the expired bond then totalShares should be decreased by bond shares amount

Proof of Concept

BondNft.claim function is called to claim bond rewards that have accrued during the time. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L168-L187

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

If bond is already expired then function distributes the amount that is accrued in next epochs after bond has expired to bonds that are not expired yet.

 if (bond.expired) {
                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];
                }
            }

BondNFT.claim can be called from BondNFT.release as well. In such situation that means that user removed his shares and indeed shares amount is removed form totalShares.

But in case if BondNFT.claim was called directly by Lock.claim before BondNFT.release was called and when bond expired that means that totalShares still contains shares of current expired bond. It should be decreased in such case to correctly distribute rewards.

Tools Used

VsCode

Recommended Mitigation Steps

You need to decrease totalShares when function is called before release lock.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L91-L122

Vulnerability details

Impact

TradingLibrary.verifyPrice doesn't check oracle for stale price. So if oracle prices are outdated it will not see that and will believe that values while checking price provided by signature.

Proof of Concept

TradingLibrary.verifyPrice function is created to validate that price signed by trusted provider is correct and that the price in chainlink oracle is +-2%. https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/utils/TradingLibrary.sol#L91-L122

    function verifyPrice(
        uint256 _validSignatureTimer,
        uint256 _asset,
        bool _chainlinkEnabled,
        address _chainlinkFeed,
        PriceData calldata _priceData,
        bytes calldata _signature,
        mapping(address => bool) storage _isNode
    )
        external view
    {
        address _provider = (
            keccak256(abi.encode(_priceData))
        ).toEthSignedMessageHash().recover(_signature);
        require(_provider == _priceData.provider, "BadSig");
        require(_isNode[_provider], "!Node");
        require(_asset == _priceData.asset, "!Asset");
        require(!_priceData.isClosed, "Closed");
        require(block.timestamp >= _priceData.timestamp, "FutSig");
        require(block.timestamp <= _priceData.timestamp + _validSignatureTimer, "ExpSig");
        require(_priceData.price > 0, "NoPrice");
        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"
                );
            }
        }
    }

When fetching price from oracle the function only uses price field. But it's possible that the price in oracle is stale. In such case protocol will still believe that price and will make price validation based on this old data.

Tools Used

VsCode

Recommended Mitigation Steps

You need to check when oracle price was updated and in case if it is old, then revert.