code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In the function addToPosition from the Trading contract the amount of open fees are handled using the _handleOpenFees function but when calling the _handleDeposit function the wrong margin is passed, in fact the _handleDeposit function gets _addMargin - _fee instead of _addMargin

So this mean that the open fees are calculated and handled but when depositing there value will not be transferred from the trader and will not be deposited in the stableVault.

Proof of Concept

The issue occurs in the addToPosition function :

File: contracts/Trading.sol Line 255-305

function addToPosition(
    uint _id,
    uint _addMargin,
    PriceData calldata _priceData,
    bytes calldata _signature,
    address _stableVault,
    address _marginAsset,
    ERC20PermitData calldata _permitData,
    address _trader
)
    external
{
    ...

    /* @audit
       fee are calculated and handled with _handleOpenFees
    */
    uint _fee = _handleOpenFees(_trade.asset, _addMargin*_trade.leverage/1e18, _trader, _trade.tigAsset, false);

    /* @audit
       But only (_addMargin - _fee) amount is deposited
    */
    _handleDeposit(
        _trade.tigAsset,
        _marginAsset,
        _addMargin - _fee,
        _stableVault,
        _permitData,
        _trader
    );
    ...
}

As you can see from the code above the _handleDeposit function receive _addMargin - _fee as new margin, this value is used to calculate the transferred amount from the trader :

File: contracts/Trading.sol Line 565-576

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

        /* @audit
           _margin == (_addMargin - _fee) is used to calculate transferred amount from trader 
           instead of the whole _addMargin value
        */

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

So because of this error the open fees amount will not be transferred from the trader and will not be deposited in the StableVault.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue correct the margin passed to the function _handleDeposit, the addToPosition function should be modified as follow :

function addToPosition(
    uint _id,
    uint _addMargin,
    PriceData calldata _priceData,
    bytes calldata _signature,
    address _stableVault,
    address _marginAsset,
    ERC20PermitData calldata _permitData,
    address _trader
)
    external
{
    ...

    /* @audit
       fee are calculated and handled with _handleOpenFees
    */
    uint _fee = _handleOpenFees(_trade.asset, _addMargin*_trade.leverage/1e18, _trader, _trade.tigAsset, false);

    /* @audit
       pass correct _addMargin amount to _handleDeposit
    */
    _handleDeposit(
        _trade.tigAsset,
        _marginAsset,
        _addMargin,
        _stableVault,
        _permitData,
        _trader
    );
    ...
}

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #659