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.

Sign up for GitHub

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

Jump to bottom

_safeMint malicious callback can corrupt _limitOrders data in Position.sol #288

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

_safeMint malicious callback can corrupt _limitOrders data in Position.sol #288

code423n4 opened this issue Dec 16, 2022 · 7 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-400 partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) 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/main/contracts/Position.sol#L148

Vulnerability details

Impact

Malicious user can use a contract call initiateLimitOrder and get a callback in the Position._safeMint, and then call cancelLimitOrder to burn the position NFT in this callback before _limitOrders and _limitOrderIndexes get updated.

In this case the burn function https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Position.sol#L264 will wrongly operate _limitOrderIndexes and _limitOrders.

Proof of Concept

todo

Tools Used

manual audit

Recommended Mitigation Steps

use _mint instead of _safeMint to avoid callback
@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
@code423n4
mookimgo issue #288
931364a
@GalloDaSballo
Copy link

Missing a clear Impact and POC, but is valid as a non CEI conformity finding

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #197

@c4-judge c4-judge reopened this Jan 16, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #400

@GalloDaSballo
Copy link

25% because missing most of the attack

@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as partial-25

@c4-judge c4-judge added partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) 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 16, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to 3 (High Risk)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-400 partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@GalloDaSballo @code423n4 @c4-judge