code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In _handleDeposit() of Trading.sol, when _tigAsset != _marginAsset and _permitData.usePermit == true, _marginAsset, an allowed token, is deposited into deposit() of StableVault.sol in exchange for tigAsset tokens. A check is done to ensure an exact amount of _margin has been added to the tigAsset balance of Trading.sol. However, the burnFrom amount is tigAsset.balanceOf(address(this)) instead of _margin. This could drastically lead to an accounting error.

Proof of Concept

File: 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();
655:            tigAsset.burnFrom(address(this), tigAsset.balanceOf(address(this)));
        } else {
657:            tigAsset.burnFrom(_trader, _margin);
        }        
    }

As can be seen from the code block above, tigAsset.balanceOf(address(this)) on line 655 is very much larger than _margin on line 657. The contract balance of tigAssert is practically burned each time the first three if blocks of the function logic return true consecutively.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider refactoring the affected code line as follows:

-            tigAsset.burnFrom(address(this), tigAsset.balanceOf(address(this)));
+            tigAsset.burnFrom(address(this), _margin);

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #336