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

Missing ownership check in memorializePositions #109

Closed
code423n4 opened this issue May 7, 2023 · 4 comments
Closed

Missing ownership check in memorializePositions #109

code423n4 opened this issue May 7, 2023 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-488 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) 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/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L170

Vulnerability details

Impact

Anyone can call memorializePositions with any user NFT and transfer ownership of any previously approved LP tokens to the PositionManager contract.
memorializePositions stamps the given NFT with the underlying liquidity positions in a given array of bucket indexes and transfers the LPB to the PositionManager contract, since this function does not check if the caller is the owner of the NFT, the LP tokens may be transfer to the PositionManager contract at an inappropriate time, impacting the position management strategy of the owner.

Proof of Concept

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L170

Tools Used

Manuel Review

Recommended Mitigation Steps

Add a check to compare the owner of the nft against the msg.sender in memorializePositions.

Assessed type

Access Control
@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 May 7, 2023
code423n4 added a commit that referenced this issue May 7, 2023
@code423n4
kodyvim issue #109
319b9cf
@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #488

@c4-judge
Copy link
Contributor

Picodes marked the issue as partial-50

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label May 29, 2023
@Picodes
Copy link

Picodes commented May 29, 2023

The impact described does not qualify for High Severity - the loss of funds scenario without external requirements is not obvious.

@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 May 30, 2023
@c4-judge
Copy link
Contributor

Picodes 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-488 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) 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
@Picodes @code423n4 @c4-judge