code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/PositionManager.sol#L178

Vulnerability details

Impact

Funds can be transferred without updating the lp position

Proof of Concept

if the indexesLength is not checked to be greater than, in a situation where the indexesLength == 0 the loop will not run and the ownership of LP will be transferred to PositionManager contract leading to possible loss of funds.

        // update pool LP accounting and transfer ownership of LP to PositionManager contract
        pool.transferLP(owner, address(this), params_.indexes);

Tools Used

VS code

Recommended Mitigation Steps

Include a require statement before the loop

        uint256 indexesLength = params_.indexes.length;
        uint256 index;

        require(indexesLength > 0, ...)

        for (uint256 i = 0; i < indexesLength; ) {
            index = params_.indexes[i];
             ...

            unchecked { ++i; }
        }

Assessed type

Context

Picodes commented 1 year ago

The function called does nothing if indexesLength == 0

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid