RewardsManager: Old bucket snapshot info cannot be deleted if positionIndexes is changed before unstaking

[code423n4]

Lines of code

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

Vulnerability details

Impact

RewardsManager's unstake() use delete stakeInfo.snapshot[positionIndexes[i]] to clear old stake snapshot info. But the positionIndexes can be changed between stake and unstake, the old stakeInfo.snapshot[positionIndexes[i]] may not be reset.

Attackers could leverage this to obtain extra rewards (interest earned will be counted from the first stake time for old positions) at the expense of other stakers.

Proof of Concept

According to the old audit results at sherlock H-1, if the old snapshot info is not reset completely, attackers could reuse the old snapshot to obtain extra rewards.

The patch iterates the stakeInfo.snapshot to reset the nested mappings.

        uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_);
        for (uint256 i = 0; i < positionIndexes.length; ) {
            delete stakeInfo.snapshot[positionIndexes[i]]; // reset BucketState struct for current position

            unchecked { ++i; }
        }

This is problematic because it calls positionManager.getPositionIndexes to get the keys of nested mappings. The results of positionManager.getPositionIndexes will change between stake and unstake. Therefore, if positionIndexes remove some old indexes, its corresponding stakeInfo.snapshot will not be reset and can be reused.

    function getPositionIndexes(
        uint256 tokenId_
    ) external view override returns (uint256[] memory) {
        return positionIndexes[tokenId_].values();
    }

If rewardsManager call reedemPositions between stake and unstake, the positionIndexes will be cleared, and unstake cannot reset the old snapshot. Please see the POC for more info.

POC: Add import "@std/console.sol"; to RewardsManager.t.sol and add this test.

    function testStakeAndUnstakeBetweenReedem() external {
        skip(10);

        // configure NFT position one
        uint256[] memory depositIndexes = new uint256[](1);
        depositIndexes[0] = 9;

        // _minterOne's depositIndexes has index '9'
        uint256 tokenIdOne = _mintAndMemorializePositionNFT({
            indexes:    depositIndexes,
            minter:     _minterOne,
            mintAmount: 1_000 * 1e18,
            pool:       address(_pool)
        });
        console.log("tokenId One: ",tokenIdOne);

        // minterOne deposits their NFT into the rewards contract
        _stakeToken({
            pool:    address(_pool),
            owner:   _minterOne,
            tokenId: tokenIdOne
        });

        // pool redeem the tokenIdOne with index [9]
        //changePrank(address(_pool));
        changePrank(address(_rewardsManager));
        IPositionManagerOwnerActions.RedeemPositionsParams memory reedemParams = IPositionManagerOwnerActions.RedeemPositionsParams(
            tokenIdOne, address(_pool), depositIndexes
        );
        address[] memory transferors = new address[](1);
        transferors[0] = address(_positionManager);
        _pool.approveLPTransferors(transferors);
        _positionManager.reedemPositions(reedemParams);

        // _minterOne unstake
        changePrank(_minterOne);
        _rewardsManager.unstake(tokenIdOne);

        // After unstake, old snapshot stakeInfo.snapshot[9] is still there
        (uint256 lpsAtStakeTime, uint256 rateAtStakeTime) = _rewardsManager.getBucketStateStakeInfo(tokenIdOne, 9);
        console.log("After unstake, snapshot 9 lpsAtStakeTime : ", lpsAtStakeTime);
        console.log("After unstake, snapshot 9 rateAtStakeTime : ", rateAtStakeTime);

    }

Run test with forge test -m testStakeAndUnstakeBetweenReedem -vvv

The result shows that after unstake, snapshot is not reset.

Running 1 test for tests/forge/unit/Rewards/RewardsManager.t.sol:RewardsManagerTest
[PASS] testStakeAndUnstakeBetweenReedem() (gas: 1267024)
Logs:
  tokenId One:  1
  After unstake, snapshot 9 lpsAtStakeTime :  1000000000000000000000
  After unstake, snapshot 9 rateAtStakeTime :  1000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 16.77s

Tools Used

Manual Review

Recommended Mitigation Steps

Store the keys of the snapshot in a separate vector and delete the snapshot according to the stored keys rather than the positionIndexes.

Assessed type

Other

[c4-sponsor]

MikeHathaway marked the issue as sponsor disputed

[MikeHathaway]

Staked positions can't be redeemed. The POC is invalid due to its use of changePrank(address(_rewardsManager));

[merc1995]

I thought _rewardsManager has the right to redeem the staked positions, if that's not true, please forgive my mistake:)

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid