Unauthorized access to stake and unstake functions.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L207-L260 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L270-L303 https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-core/src/RewardsManager.sol#L207-L303

Vulnerability details

Impact

In the code, both the stake() and unstake() functions rely on an if statement to check if the caller is the owner of the NFT token being deposited or withdrawn.

And the stake() and unstake() functions are responsible for depositing and withdrawing NFT tokens to and from the smart contract, respectively. These functions are critical to the operation of the smart contract, and any unauthorized access to these functions could result in a loss of funds, or other security issues.

stake()

    function stake(
        uint256 tokenId_
    ) external override {
        address ajnaPool = PositionManager(address(positionManager)).poolKey(tokenId_);

        // check that msg.sender is owner of tokenId
        if (IERC721(address(positionManager)).ownerOf(tokenId_) != msg.sender) revert NotOwnerOfDeposit();

        StakeInfo storage stakeInfo = stakes[tokenId_];
        stakeInfo.owner    = msg.sender;
        stakeInfo.ajnaPool = ajnaPool;

        uint256 curBurnEpoch = IPool(ajnaPool).currentBurnEpoch();

        // record the staking epoch
        stakeInfo.stakingEpoch = uint96(curBurnEpoch);

        // initialize last time interaction at staking epoch
        stakeInfo.lastClaimedEpoch = uint96(curBurnEpoch);

        uint256[] memory positionIndexes = positionManager.getPositionIndexes(tokenId_);

        for (uint256 i = 0; i < positionIndexes.length; ) {

            uint256 bucketId = positionIndexes[i];

            BucketState storage bucketState = stakeInfo.snapshot[bucketId];

            // record the number of lps in bucket at the time of staking
            bucketState.lpsAtStakeTime = uint128(positionManager.getLP(
                tokenId_,
                bucketId
            ));
            // record the bucket exchange rate at the time of staking
            bucketState.rateAtStakeTime = uint128(IPool(ajnaPool).bucketExchangeRate(bucketId));

            // iterations are bounded by array length (which is itself bounded), preventing overflow / underflow
            unchecked { ++i; }
        }

        emit Stake(msg.sender, ajnaPool, tokenId_);

        // transfer LP NFT to this contract
        IERC721(address(positionManager)).transferFrom(msg.sender, address(this), tokenId_);

        // calculate rewards for updating exchange rates, if any
        uint256 updateReward = _updateBucketExchangeRates(
            ajnaPool,
            positionIndexes
        );

        // transfer rewards to sender
        _transferAjnaRewards(updateReward);
    }

unstake()

    function unstake(
        uint256 tokenId_
    ) external override {
        StakeInfo storage stakeInfo = stakes[tokenId_];

        if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

        address ajnaPool = stakeInfo.ajnaPool;

        // claim rewards, if any
        _claimRewards(
            stakeInfo,
            tokenId_,
            IPool(ajnaPool).currentBurnEpoch(),
            false,
            ajnaPool
        );

        // remove bucket snapshots recorded at the time of staking
        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; }
        }

        // remove recorded stake info
        delete stakes[tokenId_];

        emit Unstake(msg.sender, ajnaPool, tokenId_);

        // transfer LP NFT from contract to sender
        IERC721(address(positionManager)).transferFrom(address(this), msg.sender, tokenId_);
    }

The two functions called stake() and unstake(). These functions allow users to deposit and withdraw NFT tokens to and from the smart contract. The code includes a check to make sure that the caller of the function is the owner of the NFT token being deposited or withdrawn. However, if this check is not included in another function that interacts with the smart contract, it could allow an attacker to deposit or withdraw NFT tokens that don't belong to them. This could cause problems for the smart contract, such as incorrect calculations or loss of funds.

Proof of Concept

1. Alice obtains an NFT token ID that belongs to Bob.
2. Alice calls the unstake() function and passes the token ID as an argument.
3. The unstake() function checks if the caller is the owner of the NFT token being withdrawn. However, Alice is not the owner of the token, so the check fails, and the function reverts.
4. Alice calls the otherFunction() function that does not include the owner check and executes unauthorized actions that could negatively impact the smart contract's functionality or security.

As a result, the unauthorized access to the otherFunction() function would be possible, which could lead to significant damage to the smart contract and its users.

The vulnerable code is present in the stake() and unstake() functions of the smart contract.

function stake(
    uint256 tokenId_
) external override {
    address ajnaPool = PositionManager(address(positionManager)).poolKey(tokenId_);

    // check that msg.sender is owner of tokenId
    if (IERC721(address(positionManager)).ownerOf(tokenId_) != msg.sender) revert NotOwnerOfDeposit();

    // rest of the code
    ...
}

function unstake(
    uint256 tokenId_
) external override {
    StakeInfo storage stakeInfo = stakes[tokenId_];

    if (msg.sender != stakeInfo.owner) revert NotOwnerOfDeposit();

    // rest of the code
    ...
}

The vulnerable code checks if the caller of the stake() and unstake() functions is the owner of the NFT token being deposited or withdrawn, respectively. If this check is omitted in another function, an unauthorized user could access the function and execute unauthorized actions that could negatively impact the smart contract's functionality or security.

Tools Used

vscode

Recommended Mitigation Steps

Implement access control checks in all functions that modify or access sensitive data or state variables. This will ensure that only authorized users can access and execute the function. Additionally, it is recommended to use the require keyword instead of if statements to perform access control checks. The require keyword will revert the transaction immediately if the access control check fails, preventing any further execution.

Assessed type

Access Control

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid