Tigris Trade contest - cccz's results

Lines of code

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

Vulnerability details

Impact

When the user deposits assets into the Lock, in lock(), totalLocked is correctly updated.

    function lock(
        address _asset,
        uint _amount,
        uint _period
    ) public {
        require(_period <= maxPeriod, "MAX PERIOD");
        require(_period >= minPeriod, "MIN PERIOD");
        require(allowedAssets[_asset], "!asset");

        claimGovFees();

        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        totalLocked[_asset] += _amount;
        
        bondNFT.createLock( _asset, _amount, _period, msg.sender);
    }

But this is not done in extendLock which makes totalLocked incorrect.

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

This will cause the release function to overflow when calculating totalLocked, thus preventing the user from withdrawing the asset

    function release(
        uint _id
    ) public {
        claimGovFees();
        (uint amount, uint lockAmount, address asset, address _owner) = bondNFT.release(_id, msg.sender);
        totalLocked[asset] -= lockAmount;
        IERC20(asset).transfer(_owner, amount);
    }

Consider the following scenario: User A calls Lock.lock to deposit 100 tokens for one month, where totalLocked = 100. And then calls Lock.extendLock to deposit 100 tokens again, where totalLocked == 100 since Lock.extendLock does not update totalLocked. After one month, user A calls Lock.release to withdraw the tokens, because lockAmount == 200, totalLocked - lockAmount overflows and the function fails, thus the tokens cannot be withdrawn

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

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

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L643-L653

Vulnerability details

Impact

Some tokens (like USDT on Ethereum) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

In _handleDeposit, if _marginAsset is USDT, this will cause _handleDeposit to get stuck

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

Considering that tigris is a multi-chain application, it should be adapted to USDT on ethereum

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L643-L653

Tools Used

None

Recommended Mitigation Steps

    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, 0);
            IERC20(_marginAsset).approve(_stableVault, type(uint).max);
            IStableVault(_stableVault).deposit(_marginAsset, _margin/_marginDecMultiplier);

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L19-L20

Vulnerability details

Impact

In GovNFT, setMaxBridge function is provided to set maxBridge, but this variable is not used, literally it should be used to limit the number of GovNFTs crossing chain, but it doesn't work in GovNFT.

    uint256 public maxBridge = 20;
...
    function setMaxBridge(uint256 _max) external onlyOwner {
        maxBridge = _max;
    }

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L19-L20 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L311-L313

Tools Used

None

Recommended Mitigation Steps

Consider applying the maxBridge variable

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/StableToken.sol#L13-L22

Vulnerability details

Impact

Using the burnFrom() function of stableToken, minter can burn an arbitrary amount of tokens from any address.

    function burnFrom(
        address account,
        uint256 amount
    ) 
        public 
        virtual 
        onlyMinter() 
    {
        _burn(account, amount);
    }

This is not necessary, the StableToken should be transferred from the user to the contract first and then called StableToken.burnFrom(address(this), amount).

If the private key of the owner of the StableToken is compromised, an attacker can set the minter and burn any user's StableToken

    function setMinter(
        address _address,
        bool _status
    ) 
        public
        onlyOwner()
    {
        isMinter[_address] = _status;
    }

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/StableToken.sol#L13-L22 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/StableToken.sol#L38-L46

Tools Used

None

Recommended Mitigation Steps

Update burnFrom function for only user can burn his tokens.

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L749

Vulnerability details

Impact

When Trading._handleOpenFees calls gov.distribute, gov transfers the _tigAsset from Trading to gov, which requires Trading approve _tigAsset to gov first.

        gov.distribute(_tigAsset, IStable(_tigAsset).balanceOf(address(this)));
...
    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 {
            return;
        }
    }

Since the default openFees is 0, there is no trouble in the test, but if the openFees are changed, the system will not be stuck due to the try-catch in gov.distribute, but it will cause the fees to be locked in Trading and not distributed to the GovNFT holder.

    Fees public openFees = Fees(
        0,
        0,
        0,
        0
    );
...
    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 {

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/Trading.sol#L749 https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L287-L289

Tools Used

None

Recommended Mitigation Steps

approve _tigAsset to gov before calling gov.distribute in Trading._handleOpenFees