Lack of Input Validation for tokenId Allows Setting Invalid or Unauthorized Token IDs

[CipherHawk0]

Summary

The absence of validation in the setTokenId function will cause a high impact on the staking mechanism for the protocol as an attacker with the SETTER_ROLE can set arbitrary tokenId values, leading to unauthorized staking or misallocation of rewards.

Root Cause

In SolidlyV2AMO.sol, the setTokenId function allows authorized addresses with the SETTERROLE to set the tokenId used for staking in the gauge without validating whether the provided tokenId is valid or authorized.

Relevant Code Snippet:

function setTokenId(uint256 tokenId_, bool useTokenId_) public override onlyRole(SETTER_ROLE) {
    tokenId = tokenId_;
    useTokenId = useTokenId_;
    emit TokenIdSet(tokenId, useTokenId);
}

Internal pre-conditions

1. An account with the SETTER_ROLE needs to call setTokenId to set the tokenId.

2. The tokenId_ being set is arbitrary and not validated against any authorization criteria.

External pre-conditions

1. The veNFT contract (if applicable) should have mechanisms to verify the validity and ownership of tokenId_.

2. External contracts (gauge, etc.) should have secure interfaces that handle unexpected tokenId values gracefully.

Attack Path

1. The attacker, possessing the SETTER_ROLE, calls the setTokenId function.

2. The attacker sets tokenId_ to an arbitrary or unauthorized value that does not correspond to a legitimate veNFT holder.

3. The protocol's staking and reward mechanisms interact with the invalid tokenId, causing misallocation of rewards or unauthorized staking.

4. Rewards intended for legitimate stakers may be redirected or become inaccessible, undermining the protocol's integrity.

Impact

The protocol suffers a high risk of misallocating rewards and disrupting the staking mechanism, leading to financial losses for legitimate users and loss of trust in the protocol's fairness and reliability.

PoC

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import "./SolidlyV2AMO.sol";

contract AttackerContract {
    SolidlyV2AMO amo;

    constructor(address amoAddress) {
        amo = SolidlyV2AMO(amoAddress);
    }

    function exploit() public {
        // Assume the attacker has SETTER_ROLE
        // Set an invalid tokenId that does not belong to a legitimate veNFT holder
        amo.setTokenId(999999, true);
    }
}

Mitigation

1. Implement tokenId Validation:

Add checks to ensure that the provided tokenId_ is valid and authorized before setting it.

function setTokenId(uint256 tokenId_, bool useTokenId_) public override onlyRole(SETTER_ROLE) {
    require(_isValidTokenId(tokenId_), "Invalid tokenId");
    tokenId = tokenId_;
    useTokenId = useTokenId_;
    emit TokenIdSet(tokenId, useTokenId);
}

function _isValidTokenId(uint256 tokenId_) internal view returns (bool) {
    // Implement logic to verify tokenId validity, e.g., check ownership or existence
    return IVeNFT(veNFT).exists(tokenId_);
}

2. Restrict SETTER_ROLE Privileges:

Limit the number of addresses with the SETTER_ROLE to reduce the risk of unauthorized parameter manipulation.

3. Implement Multi-Signature Controls:

Require multiple approvals for setting critical parameters like tokenId, adding an additional layer of security.

4. Regular Security Audits:

Conduct periodic audits to ensure that all role-based functions have adequate validation and security measures.

5. Use Immutable Token IDs Where Possible:

If feasible, design the contract to use immutable tokenId values that cannot be changed post-deployment, preventing unauthorized manipulation.