code-423n4 / 2023-05-ajna-findings

2 stars 0 forks source link

mint() function: an attacker can mint multiple position NFTs for one or more legit Ajna users who have LP in Ajna pools. This should not be possible. #463

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/d80daab705a066828ef1c5d9ba85f315c7c932db/ajna-core/src/PositionManager.sol#L227-L241

Vulnerability details

Impact

The current implementation of the mint function does not ensure that only the owner of a liquidity pool (LP) deposit can mint position NFTs. As a result, an attacker can mint multiple position NFTs on behalf of legitimate Ajna users who have LP in Ajna pools. This could lead to a distortion of the NFT marketplace, as an attacker can inflate the number of NFTs associated with a given user. It also opens up possibilities for fraudulent activities where an attacker could potentially mint NFTs in a legitimate user's name without their consent.

Proof of Concept

In the current implementation, the mint function can be invoked without any checks on whether the caller is the owner of the specified LP deposit.

Here's an example of how an attacker can exploit this: mint({pool: somePool, poolSubsetHash: someHash, recipient: someAddress});

The caller of the function can mint a new NFT and assign it to the recipient, a valid Ajna pool user(lender/borrower). This NFT is minted without the recipient's consent and could be misused by the attacker.

Tools Used

VSC.

Recommended Mitigation Steps

To prevent unauthorized minting of NFTs on behalf of other users, it is recommended to introduce a check in the mint function to ensure that the caller is the owner of the specified LP deposit. This could be done through a modifier that checks the LP ownership before the NFT is minted.

Here's a simple example of how such a check could be implemented:

modifier isOwnerOfLp(address minter, address pool) { require(isLpOwner(minter, pool), "Not owner of LP deposit"); _; }

function mint(MintParams calldata params) external override isOwnerOfLp(msg.sender, params.pool) nonReentrant returns (uint256 tokenId_) { // ... rest of the mint function }

In the above code, isLpOwner is a hypothetical function that checks if an address is the owner of an LP deposit in a specific pool. Should replace it with the actual function used in the system to check LP ownership.

This way, only addresses that own an LP deposit in the specified pool can mint NFTs, preventing unauthorized minting of NFTs on behalf of other users.

Assessed type

Access Control

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Insufficient quality