Reentrancy Vulnerability When Transferring Pool Tokens in PoolOwnership.sol

The PoolOwnership.sol contract implements ERC721 transfers for ownership of staking pools. In the following functions:

function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual override {
    super._safeTransfer(from, to, tokenId, data);
    StakingPool(address(uint160(tokenId))).moveStakeToSelfStake(to);
}

function transferFrom(address from, address to, uint256 tokenId) public virtual override {
    super.transferFrom(from, to, tokenId);
    StakingPool(address(uint160(tokenId))).moveStakeToSelfStake(to);
}

The contract first calls the standard ERC721 transfer logic via super._safeTransfer or super.transferFrom. This transfers ownership of the NFT, which represents control of a StakingPool, to the address. Only after the transfer is completed, the moveStakeToSelfStake(to) function is called on the StakingPool contract to update staking balances.

This ordering introduces a critical vulnerability:

• If the address is a smart contract, it can implement onERC721Received.
• Within onERC721Received, the contract can perform arbitrary actions on the StakingPool, since it is now recognized as the owner of the pool.
• However, the internal staking state has not yet been updated. Specifically, the stakes[to] balance has not yet been migrated to selfStake via moveStakeToSelfStake.

function moveStakeToSelfStake(address staker) public {
    require(msg.sender == address(ownershipNFT), "Only NFT contract can call");
    if (stakes[staker] > 0) {
        selfStake += stakes[staker];
        stakes[staker] = 0;
    }
}

Because the transfer happens before calling this function, the recipient has a temporary window where they own the pool NFT, their stakes[to] is still intact, and selfStake has not yet been increased. This opens the door to reentrancy attacks or manipulation of the staking logic. The new owner could call other methods on the StakingPool using an outdated balance, trigger a withdraw, delegate, or any operation that reads from stakes[to] before it's cleared, or exploit this outdated state to double-count, bypass checks, or extract unintended rewards.

Since the NFT transfer includes an external call (onERC721Received), and the state update occurs after that call, the contract violates the checks-effects-interactions pattern, a core Solidity security principle to prevent reentrancy and inconsistent state issues.

A test demonstrated the vulnerability: an attacker contract receiving the pool NFT could read stakingPool.currentStake(address(this)) during the onERC721Received hook and observe the not-yet-migrated stake balance, confirming the reentrancy window.

In a real attack, this access would allow state manipulation and possible double spending.

Reorder the logic: always update internal contract state before performing external calls or transferring ownership. In this case, moveStakeToSelfStake(to) should be called before the ERC721 token is transferred.

function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual override {
    StakingPool(address(uint160(tokenId))).moveStakeToSelfStake(to);
    super._safeTransfer(from, to, tokenId, data);
}