bitsCrunch - NFTStaking - bitsCrunch

The ContributorNFTStaking contract contains a mechanism where the contract owner can indefinitely extend the lock period for staked NFTs through the setNftLockPeriod function. By continuously increasing the nftLockPeriod_, the owner can exploit the check _checkAndRevert(epochPassed >= nftLockPeriod_, "Staking: NFT is Locked"); to ensure this condition always fails within the unstakeNFT function, effectively preventing users from unstaking their NFTs.

This functionality grants the owner undue control over users' assets.

The following test case was added to nfftStaking.test.js:

it("Owner prevents users from withdrawing their NFT", async () => {
            const { staking, bitscrunchnft, deployer, contributor1 } = await loadFixture(deployStakingFixture);
            const tokenId = 1
            await mintAndStakeNFT(bitscrunchnft, deployer, staking, contributor1, tokenId)
            const blocksToMine = (EPOCH_MANAGER.EPOCH_LENGTH * (LOCK_PERIOD + 1))
            await mineBlocks(blocksToMine);

            // User should be able to unstake NFT but owner front-run and add period
            let currentLockPeriod = await staking.getParamStates();
            await staking.setNftLockPeriod(currentLockPeriod + 10)
            
            // User try to unstake token but the tx revert
            await expect(staking.connect(contributor1).unstakeNFT())
                .to.be.revertedWithCustomError(staking,"RevertedWithMessage")
                .withArgs("Staking: NFT is Locked");

            // Owner re put normal lock period preventing user from unstaking
            await staking.setNftLockPeriod(currentLockPeriod)
        }); 

Introduce an unlockEpoch attribute within the ContributorNft struct to determine the specific epoch when an NFT becomes eligible for unstaking. This attribute is to be set upon staking based on the current epoch plus the current nftLockPeriod_, thus decoupling individual NFT unstake eligibility from subsequent modifications to the global lock period.

Adjustments:

1. Struct Modification: Add uint256 unlockEpoch; to the ContributorNft struct.

2. Staking Logic: In stakeNFT, set currentContributorNFT.unlockEpoch to currentEpoch + nftLockPeriod_ upon NFT staking.

3. Unstaking Logic: In unstakeNFT, replace the global lock period check with a validation against unlockEpoch, allowing unstaking if the current epoch is greater than or equal to unlockEpoch.

Remediation Plan

RISK ACCEPTED: The bitsCrunch team will check if an upper bound for lock period is necessary and update it

The ContributorNFTStaking contract's stakeNFT function currently only checks for individual NFT approval via getApproved and does not account for global approval set through setApprovalForAll. This oversight renders the contract non-compliant with scenarios where users have granted universal approval to manage their NFTs, leading to failed transactions even when setApprovalForAll has been invoked favorably towards the staking contract.

This limitation restricts user flexibility in managing NFT approvals, potentially deterring participation in the staking process due to the inconvenience of having to approve each NFT individually.

The following test case was added to nfftStaking.test.js:

async function mintAndStakeNFTWithApproveForAll(nft,deployer,staking,contributor,tokenId) {
        await mintToken(nft, deployer,contributor, tokenId )
        await nft.connect(contributor).setApprovalForAll(staking.address, true)
        await staking.connect(contributor).stakeNFT(tokenId)
    }


it("Reverts if NFT is approvedForAll", async () => {
            const { staking, bitscrunchnft , deployer,contributor1 } = await loadFixture(deployStakingFixture);
            const tokenId = 123
            await expect(mintAndStakeNFTWithApproveForAll(bitscrunchnft,deployer,staking,contributor1,tokenId))
               .to.be.revertedWithCustomError(staking,"RevertedWithMessage")
               .withArgs("Staking: NFT is not approved");
        });

Modify the _isNFTApproved function to include a check for global approval via isApprovedForAll, in addition to the existing check for individual approval. The revised function should resemble:

function _isNFTApproved(uint256 _tokenId) internal view returns(bool) {
    return (IERC721Upgradeable(bitscrunchnft_).getApproved(_tokenId) == address(this) || 
            IERC721Upgradeable(bitscrunchnft_).isApprovedForAll(_msgSender(), address(this)));
}

Remediation Plan

SOLVED : The bitsCrunch team Added validation recommended (check either approveForAll or Approved is OK)

The totalNftStaked_ variable in the ContributorNFTV1Storage contract is never updated within any function, resulting in its value persistently remaining at 0. This oversight implies that the contract fails to accurately track and report the total number of NFTs staked, undermining the integrity of staking data and potentially affecting functionalities reliant on this metric.

Implement logic within the stakeNFT and unstakeNFT functions to increment and decrement totalNftStaked_ respectively, upon successful staking or unstaking of an NFT. This adjustment ensures the variable accurately reflects the current total of staked NFTs.

function stakeNFT(uint256 _tokenId) external nonReentrant {
    // .. //
    totalNftStaked_ += 1;
    // .. //
}

function unstakeNFT() external nonReentrant {
    // .. //
    totalNftStaked_ -= 1;
    // .. //
}

Remediation Plan

SOLVED: The bitsCrunch team now update the state in stakeNFT and unstakeNFT functions

The ContributorNFTStaking contract currently permits the contract owner to set the NFT lock period (nftLockPeriod_) to any arbitrary value without an upper bound. This design choice grants the owner the capability to indefinitely extend the lock period, potentially restricting users from unstaking their NFTs for an unreasonable duration.

    function setNftLockPeriod(uint256 _lockPeriod) external onlyOwner {
        _checkAndRevert(_lockPeriod > 0, "Staking: Lock Period Cant Be Zero");
        uint256 previousLockPeriod = nftLockPeriod_;
        _setNftLockPeriod(_lockPeriod);
        emit LockPeriodUpdated(_lockPeriod, previousLockPeriod);
    }

To mitigate this risk and enhance the contract's robustness, it is recommended to implement a maximum threshold for the NFT lock period. This can be achieved by defining a constant MAX_NFT_LOCK_PERIOD and enforcing this limit within the setNftLockPeriod function. Such a constraint ensures that the lock period remains within a reasonable range, aligning with the contract's intended functionality and user expectations.

// Define a maximum lock period constant
uint256 public constant MAX_NFT_LOCK_PERIOD = <desired_maximum_period>;

function setNftLockPeriod(uint256 _lockPeriod) external onlyOwner {
    require(_lockPeriod > 0 && _lockPeriod <= MAX_NFT_LOCK_PERIOD, "Invalid lock period");
    uint256 previousLockPeriod = nftLockPeriod_;
    nftLockPeriod_ = _lockPeriod;
    emit LockPeriodUpdated(_lockPeriod, previousLockPeriod);
}

Remediation Plan

ACKNOWLEDGED: The bitsCrunch team will internally check if an upper bound for lock period is necessary and update it.

The initialize function in the ContributorNFTStaking contract lacks validation checks for zero-address inputs for critical parameters such as _bitscrunchnft (the NFT contract address) and _epochManager (the Epoch Manager contract address). Even if these addresses are checked during script deployment, this oversight may lead to the contract being initialized with invalid addresses, rendering the staking mechanism non-operational and potentially compromising contract integrity.

Incorporate address validation checks within the initialize function to ensure non-zero addresses are provided for both the NFT contract and the Epoch Manager contract. This can be achieved by adding the following conditions:

require(_bitscrunchnft != address(0), "NFT contract address cannot be zero");
require(_epochManager != address(0), "Epoch Manager address cannot be zero");

Remediation Plan

SOLVED: The bitsCrunch team added address(0) checks.

The stakeNFT and unstakeNFT functions within the ContributorNFTStaking contract exhibit suboptimal gas efficiency due to repeated storage operations performed on the ContributorNft struct. In Solidity, storage operations are costly in terms of gas usage, significantly impacting the overall transaction cost.

In both functions, the currentContributorNFT struct, which is stored in a mapping, is modified multiple times. Each modification directly affects state storage, leading to increased gas consumption.

A more gas-efficient approach involves loading the ContributorNft struct into memory at the function's start, performing all necessary modifications on this memory copy, and then updating the storage mapping only once at the function's end. This method minimizes storage operations, thereby reducing gas costs.

function stakeNFT(uint256 _tokenId) external nonReentrant {
    address staker = _msgSender();
    ContributorNft memory currentContributorNFT = contributorNftInfo_[staker];

    _checkAndRevert(!currentContributorNFT.isStaked, "Staking: Already Staked NFT");
    _checkAndRevert(_isNFTApproved(_tokenId), "Staking: NFT is not approved");
    
    uint256 currentEpoch = epochManager_.currentEpoch();
    currentContributorNFT.nftId = _tokenId;
    currentContributorNFT.isStaked = true;
    currentContributorNFT.lastStakedEpoch = currentEpoch;

    contributorNftInfo_[staker] = currentContributorNFT; // Single storage operation
    TokenUtils.pullNFT(bitscrunchnft_, staker, _tokenId);
    emit StakeNFT(staker, _tokenId, currentEpoch);
}

function unstakeNFT() external nonReentrant {
    address staker = _msgSender();
    ContributorNft memory currentContributorNFT = contributorNftInfo_[staker];

    _checkAndRevert(currentContributorNFT.isStaked, "Staking: Not Staked NFT Yet");
    uint256 epochPassed = epochManager_.epochsSince(currentContributorNFT.lastStakedEpoch);
    _checkAndRevert(epochPassed >= nftLockPeriod_, "Staking: NFT is Locked");

    currentContributorNFT.isStaked = false;
    contributorNftInfo_[staker] = currentContributorNFT; // Single storage operation

    TokenUtils.pushNFT(bitscrunchnft_, staker, currentContributorNFT.nftId);
    emit UnStakeNFT(staker, currentContributorNFT.nftId, currentEpoch);
}

Remediation Plan

SOLVED : The bitsCrunch team will necessary modifications recommended above to be more gas efficient but did not implement it for now

In the GovernanceController contract, the onlyManager modifier utilizes the error message "Governance:RTO," which stands for "Restricted to Owner." This message is identical to the one used in the onlyOwner modifier, potentially leading to ambiguity since the onlyManager modifier is intended to authorize both managers and the owner. Such uniformity in error messaging could cause confusion regarding the source of access denial, obscuring the distinction between restrictions applied to owners versus managers.

To enhance clarity and improve error message specificity, it is advised to update the error message associated with the onlyManager modifier. A distinct error message will better reflect the broader access control intended by this modifier, which encompasses both managers and the owner.

modifier onlyManager() {
    _checkAndRevert(
        hasRole(MANAGER_ROLE, _msgSender()) || hasRole(DEFAULT_ADMIN_ROLE, _msgSender()),
        "Governance:RTMO" // "Restricted to Manager/Owner"
    );
    _;
}

Remediation Plan

ACKNOWLEDGED: The bitsCrunch team did not update the code now as it is used in multiple other deployed contracts