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

2 stars 0 forks source link

mint() function: Rogue lenders/attackers could mint multiple/endless position NFTs for their SAME Ajna pool deposits/LPs, when they're supposed to be able to mint only one position NFT per lender per LP per pool. #466

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 allows a lender to mint multiple position NFTs for the same Ajna pool deposit. This could lead to an inflation of NFTs and potentially disrupt the system's reward distribution, as the lender could stake these multiple NFTs and claim more rewards than they should. This could also result in an unfair advantage for such lenders over others who abide by the intended one NFT per lender per pool per total LP rule.

Proof of Concept

The mint function currently does not check whether an NFT has already been minted for the same Ajna pool deposit by the same lender. This allows the same lender to mint multiple NFTs for the same pool deposit.

Here's an example of how a rogue lender could exploit this: // First NFT mint({pool: myPool, poolSubsetHash: someHash, recipient: myAddress}); // Second NFT for the same pool deposit mint({pool: myPool, poolSubsetHash: someHash, recipient: myAddress});

The lender can repeat this process and mint an arbitrary number of NFTs for the same pool deposit, which is not the intended behavior.

Tools Used

VSC.

Recommended Mitigation Steps

To prevent a lender from minting multiple NFTs for the same Ajna pool deposit, it is recommended to introduce a mapping that keeps track of whether a position NFT has already been minted for a specific lender and pool deposit. In the mint function, check this mapping before minting a new NFT, and revert the transaction if an NFT for the same pool deposit has already been minted by the same lender.

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

// Mapping to track whether a position NFT has already been minted // for a specific lender and pool deposit mapping(address => mapping(address => bool)) private _minted;

function mint(MintParams calldata params) external override nonReentrant returns (uint256 tokenId) { // Check if a position NFT has already been minted for this lender and pool deposit require(!minted[msg.sender][params.pool], "Position NFT already minted for this pool deposit");

// ... rest of the mint function

// Mark that a position NFT has been minted for this lender and pool deposit
_minted[msg.sender][params_.pool] = true;

}

This way, a lender can only mint one position NFT per Ajna pool deposit, as intended, preventing the minting of multiple NFTs for the same deposit.

Assessed type

Other

Picodes commented 1 year ago

This is essentially the same "vulnerability" as #454 and #459 by the same warden. Please do not spam and focus on higher quality impact descriptions and on doing 1 issue per report.

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid