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

Adding to an existing position results in leakage of margin in stable vault #488

Closed
code423n4 opened this issue Dec 16, 2022 · 2 comments
Closed
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

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

'addToPosition' function adds additional margin to an existing trade. Margin value sent to '_handleDeposit' function sends margin adjusted for 'fees' instead of sending full margin. This results in a lesser margin withdrawn from traders account - since this directly impacts the margin amount deposited in stable vault, I have assigned 'HiGH' risk to this finding

In Line 275, margin value sent to '_handleDeposit' is '_add Margin -_fee' instead of '_addMargin'.

In Line 651, note that the 'margin' amount is transfered from trader to this contract. Since we sent a 'margin adjusted for fees', a lesser margin will be deducted from trader account.

This messes with the fees calculations as tgUSD is minted to govNFT holders with no collateral backing the tokens. On a cumulative basis, when traders add Margin, this shortfall keeps growing

Proof of Concept

Bob is a trader with an existing position on ETH/USDT pair. Bob adds an additional 100k USDT as margin. At a 0.1% mint fee, Bob will have a fees of 100$. Current contract is only sending 99,900 USDT margin amount to '_handleDeposit' which subsequently withdraws this amount from trader account.

100$ that was supposed to be rewarded to Gov NFT holders never made it to the protocol. Essentially the tgUSD minted to referrer/ bots is unbacked because of this shortfall

Tools Used

Recommended Mitigation Steps

Strangely in Line 180 , when a new market order is created, logic is correctly handled. '_tradeinfo.margin' is sent to '_handleDeposit' instead of '_marginAfterfees'. However exact same logic in 'addPosition' is not followed.

I recommend to pass '_addMargin' instead of '_addMargin -_fee' to the 'handleDeposit' function in line 275 for correct accounting

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly 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-Staff added a commit that referenced this issue Jan 6, 2023
@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 22, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as satisfactory

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
Projects
None yet
Development

No branches or pull requests

2 participants