Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fees are not collected from traders in addToPosition #433

Closed
code423n4 opened this issue Dec 16, 2022 · 3 comments
Closed

Fees are not collected from traders in addToPosition #433

code423n4 opened this issue Dec 16, 2022 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-659 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

The protocol doesn't receive fees when margin is added via the addToPosition function. An equal amount of tigUSD is minted even though it's not backed by margin.

Proof of Concept

The protocol takes fees from margin provided by users: for example, when opening a market order:

  1. fees are calculated on the total order size (margin * leverage): Trading.sol#L178;
  2. full margin is transferred from the trader (fees are not subtracted): Trading.sol#L180;
  3. accounted margin excludes fess: Trading.sol#L183-L194 (see _marginAfterFees).

Thus, traders provide full margin when opening orders and fees are subtracted from accounted margin. If trader closes an order, they'll receive margin - fees.

The protocol also allows traders to add margin to existing positions at a different price, via the addToPosition function (Trading.sol#L255). This function, however, subtracts fees from margin before transferring margin from traders:

_handleDeposit(
    _trade.tigAsset,
    _marginAsset,
    _addMargin - _fee, // @audit trader won't pay fees
    _stableVault,
    _permitData,
    _trader
);

As a result, the protocol doesn't receive fees when margin is added via addToPosition.

Tools Used

Manual review

Recommended Mitigation Steps

Consider not subtracting fees from _addMargin in addToPosition when calling _handleDeposit.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 16, 2022
code423n4 added a commit that referenced this issue Dec 16, 2022
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #659

@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to 3 (High Risk)

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jan 18, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-659 satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

2 participants