Proof of Play / Pirate Nation - Proof of Play

As per the documentation, in the contract StakingSystem, when pirates unstake their items, there should be a 50% chance for all of their gold being stolen by the navies.

This logic is implemented in the functions _claimGameNFTStakeRewards() and fulfillRandomWordsCallback().

In the _claimGameNFTStakeRewards() function, the variable balance is set initially to zero, but then it is never updated with the number of Pirate Ships staked which means that every VRFRequest.balance will be zero:

StakingSystem.sol

function _claimGameNFTStakeRewards(
    address nftContract,
    uint256 nftTokenId,
    bool unstake
) internal {
    address relevantAccount = tx.origin;

    require(
        IGameNFT(nftContract).ownerOf(nftTokenId) == relevantAccount,
        "ORIGIN_NOT_OWNER_OF_NFT: Origin is not the owner of the specified NFT"
    );

    GameNFTStake storage nftStake = stakedNFTs[nftContract][nftTokenId];
    require(
        nftStake.reservationId != 0,
        "NFT_NOT_STAKED: NFT has not been staked"
    );

    uint256 goldOwed = 0;
    uint256 xpOwed = 0;
    uint256 balance = 0;

    for (uint256 i = 0; i < nftStake.gameItemStakes.length; i++) {
        (
            uint256 goldFromShip,
            uint256 xpFromShip
        ) = _claimGameItemStakeRewards(nftStake.gameItemStakes[i], unstake);
        goldOwed += goldFromShip;
        xpOwed += xpFromShip;
    }

    // Grant XP
    if (xpOwed > 0) {
        ITraitsConsumer(nftContract).incrementTrait(
            nftTokenId,
            TraitsLibrary.XP_TRAIT_ID,
            xpOwed
        );
    }

    if (unstake) {
        // Figure out final amount of gold the player earns with some randomness
        uint256 requestId = _requestRandomWords(1);
        vrfRequests[requestId] = VRFRequest({
            account: relevantAccount,
            goldOwed: goldOwed,
            balance: balance,
            nftContract: nftContract,
            nftTokenId: nftTokenId
        });

        // Release hold on NFT
        _lockingSystem().removeNFTReservation(
            nftContract,
            nftTokenId,
            nftStake.reservationId
        );

        // Delete the stake
        delete stakedNFTs[nftContract][nftTokenId];

        // Emit unstaked event
        emit NFTUnstaked(relevantAccount, nftContract, nftTokenId, requestId);
    } else {
        // Mint gold rewards for user
        if (goldOwed > 0) {
            goldToken.mint(relevantAccount, goldOwed);
        }

        emit NFTRewardsClaimed(
            relevantAccount,
            nftContract,
            nftTokenId,
            unstake,
            goldOwed
        );
    }
}

\color{black} \color{white}

This means that when the fulFillRandomWordsCallback() function is called by Chainlink VRF:

1. uint256 coinFlips = randomness % (2**request.balance);
2. (2**request.balance) = (2**0) = 1, (considering that request.balance will always be 0 here)
3. uint256 coinFlips = randomness % 1, (any number divided by 1 will always have as remainder 0)
4. coinFlips will always be 0
5. if coinFlips is 0, numStolen will never be increased and will always be 0, hence: Navies will never steal pirate's gold when they are unstaked.

StakingSystem.sol

function fulfillRandomWordsCallback(
    uint256 requestId,
    uint256[] memory randomWords
) external override onlyRole(GameRegistryLibrary.RANDOMIZER_ROLE) {
    VRFRequest storage request = vrfRequests[requestId];
    address account = request.account;

    if (account != address(0)) {
        uint256 randomness = randomWords[0];

        // This should not overflow since the balance is determined by gameplay logic
        // and the user wont have more than 256 ships per stake
        uint256 coinFlips = randomness % (2**request.balance); // 50% chance of stealing gold per ship staked
        uint256 numStolen = 0;
        uint256 goldOwed = request.goldOwed;

        while (coinFlips > 0) {
            if (coinFlips & 1 == 1) {
                numStolen++;
            }
            coinFlips = coinFlips >> 1;
        }

        if (numStolen > 0) {
            uint256 owedToNavy = numStolen * (goldOwed / request.balance);
            _payNavyTax(owedToNavy);
            goldOwed = goldOwed - owedToNavy;
        }

        // Mint gold rewards for user
        if (goldOwed > 0) {
            goldToken.mint(account, goldOwed);
        }

        // Emit event
        emit NFTRewardsClaimed(
            account,
            request.nftContract,
            request.nftTokenId,
            true,
            goldOwed
        );

        delete vrfRequests[requestId];
    }
}

\color{black} \color{white}

In the image below, 20 Pirate Ships are unstaked which are staked to Pirate ID 60 (command_rank of PirateId 60 = 4):

SOLVED: The Proof of Play team fixed the issue and now correctly updates the balance local variable before making the _requestRandomWords(1) call:

StakingSystem.sol

uint256 balance = 0;

for (uint256 i = 0; i < nftStake.gameItemStakes.length; i++) {
    GameItemStake storage gameItemStake = nftStake.gameItemStakes[i];
    (
        uint256 goldFromShip,
        uint256 xpFromShip
    ) = _claimGameItemStakeRewards(gameItemStake, unstake);
    goldOwed += goldFromShip;
    xpOwed += xpFromShip;
    balance += gameItemStake.balance;
}

\color{black} \color{white}

In the LootSystem contract, the function batchGrantLootWithoutRandomness() was implemented:

LootSystem.sol

/**
 * @inheritdoc ILootSystem
 */
function batchGrantLootWithoutRandomness(
    address to,
    Loot[] calldata loots,
    uint8 amount
) external {
    for (uint8 idx; idx < loots.length; ++idx) {
        Loot memory loot = loots[idx];
        if (loot.lootType == LootType.LOOT_TABLE) {
            revert InvalidLootType(loot.lootType);
        } else {
            _mintLoot(to, loot, loot.amount * amount);
        }
    }
}

This is an external function that allows to grant the given user loot(s) in batches without any randomness. But this function lacks an access control mechanism and could be called by any user.

A malicious user could call this function to continuously mint himself any type of loot.

SOLVED: The Proof of Play team fixed the issue and added access control to the LootSystem.batchGrantLootWithoutRandomness() function:

LootSystem.sol

/**
 * @inheritdoc ILootSystem
 */
function batchGrantLootWithoutRandomness(
    address to,
    Loot[] calldata loots,
    uint8 amount
) external nonReentrant onlyRole(GAME_LOGIC_CONTRACT_ROLE) {
    for (uint8 idx; idx < loots.length; ++idx) {
        Loot memory loot = loots[idx];
        if (loot.lootType == LootType.LOOT_TABLE) {
            revert InvalidLootType(loot.lootType);
        } else {
            _mintLoot(to, loot, loot.amount * amount);
        }
    }
}

Fixed Commit ID: aa8cddf72d4e6ec949f0a5514ec423eed4b2f60f

In the contract StakingSystem, the function claimNavyStakeRewards() is used to claim the Gold Tokens rewards for the Navy ships staked. This function calls the _claimGameItemStakeRewards() to calculate the goldOwed:

StakingSystem.sol

function claimNavyStakeRewards(
    address account,
    uint256[] calldata stakeIndexes,
    bool unstake
) external whenNotPaused nonReentrant {
    require(
        tx.origin == account,
        "USER_CALLER_ONLY: Only EOA can claim for their own account"
    );

    GameItemStake[] storage stakedItems = navyItemStakes[account];

    uint256 goldOwed = 0;
    uint256 lastStakeIndex = stakedItems.length;
    for (uint256 idx = 0; idx < stakeIndexes.length; idx++) {
        uint256 stakeIndex = stakeIndexes[idx];
        require(
            stakeIndex < lastStakeIndex,
            "STAKE_INDEX_MUST_DECREASE: StakeIndexes must be sorted in descending order and be within bounds"
        );

        // Claim rewards
        (uint256 goldFromShip, ) = _claimGameItemStakeRewards(
            stakedItems[stakeIndex],
            unstake
        );
        goldOwed += goldFromShip;

        // Emit event
        emit NavyRewardsClaimed(
            account,
            stakedItems[stakeIndex].tokenId,
            stakedItems[stakeIndex].balance,
            unstake,
            goldOwed
        );

        // If unstaking, remove from array by swapping to end and popping
        if (unstake) {
            if (stakedItems.length > 1) {
                stakedItems[stakeIndex] = stakedItems[
                    stakedItems.length - 1
                ];
            }
            stakedItems.pop();
        }

        lastStakeIndex = stakeIndex;
    }

    // Mint gold rewards for user
    if (goldOwed > 0) {
        goldToken.mint(account, goldOwed);
    }
}

\color{black} \color{white}

The _claimGameItemStakeRewards() itself calls the _calculateGameItemStakeRewards() function and then, in the line 794, the stake.value is updated to totalTaxInGoldPerRank:

StakingSystem.sol

function _claimGameItemStakeRewards(
    GameItemStake storage stake,
    bool unstake
) internal returns (uint256 goldOwed, uint256 xpOwed) {
    address account = _msgSender();

    (goldOwed, xpOwed) = _calculateGameItemStakeRewards(stake);
    bool pirateShip = GameHelperLibrary._isPirateShip(
        gameItems,
        stake.tokenId
    );

    if (pirateShip) {
        require(
            !unstake ||
                ((block.timestamp - stake.value) >= MINIMUM_TO_EXIT),
            "STAKE_NOT_COMPLETE: Must be staked for minimum time before unstaking"
        );

        // If pirate is just collecting, there is a flat tax on their earnings
        if (!unstake) {
            uint256 owedToNavy = (goldOwed * GOLD_CLAIM_TAX_PERCENTAGE) /
                100;
            _payNavyTax(owedToNavy); // Pay tax to navy
            goldOwed = goldOwed - owedToNavy; // Rest goes to owner
        }
    } else {
        if (unstake) {
            uint8 rank = GameHelperLibrary._rankForNavy(
                gameItems,
                stake.tokenId
            );
            totalNavyRankStaked -= rank; // Remove rank from total staked
        }
    }

    if (unstake) {
        // Release reservation on items
        _lockingSystem().removeItemReservation(
            account,
            address(gameItems),
            stake.tokenId,
            stake.reservationId
        );

        // Emit event
        emit GameItemUnstaked(account, stake.tokenId, stake.balance);
    } else {
        // Reset collection timer
        stake.value = uint80(
            pirateShip ? block.timestamp : totalTaxInGoldPerRank
        );
    }
}

\color{black} \color{white}

Solidity 0.8 is introducing type checking for arithmetic operations, but not for type casting. Because of this, an overflow may occur in the Line 794 if totalTaxInGoldPerRank is higher than 2**80-1 = 1208925819614629174706175. If that overflow occurs any user with a Navy staked would be able to call claimNavyStakeRewards() as many times as he wanted with no time restriction minting with every call the same amount of Gold Tokens.

In the image below, we can see how the user2 keeps increasing his GoldToken balance after every claimNavyStakeRewards():

SOLVED: The Proof of Play team fixed the issue by updating the GameItemStake.value to uint128. On the other hand, OpenZeppelin's SafeCast library is now used for all the castings.

In the contract RaffleMintV1, the function withdrawNonRaffleProceeds() is vulnerable to reentrancy as it updates the nonRaffleWithdrawableProceeds after the payable(_msgSender()).call{value: proceeds}("");:

RaffleMintV1.sol

/* @notice Allows contract owner to withdraw proceeds of mints initiated after raffle */
function withdrawNonRaffleProceeds() external onlyOwner {
    // Ensure there are proceeds to claim
    require(
        nonRaffleWithdrawableProceeds > 0,
        "NONRAFFLE_PAYOUT_EMPTY: No proceeds available to claim"
    );

    uint256 proceeds = nonRaffleWithdrawableProceeds;

    // Pay owner proceeds
    (bool sent, ) = payable(_msgSender()).call{value: proceeds}("");
    require(sent == true, "NONRAFFLE_PAYOUT_UNSUCCESFUL");

    // Proceeds are now claimed so clear amount
    nonRaffleWithdrawableProceeds = 0;

    // Emit successful proceeds claim
    emit NonRaffleProceedsClaimed(_msgSender(), proceeds);
}

\color{black} \color{white}

By exploiting this reentrancy, the contract owner could drain all the Ether of the smart contract and users would not be able to get a refund of their losing raffle tickets.

SOLVED: The Proof of Play team fixed the issue by adding the nonReentrant modifier to the withdrawNonRaffleProceeds() function.

In the QuestSystem contract every time a quest is completed Chainlink VRF is used:

QuestSystem.sol

function completeQuest(uint64 activeQuestId)
    external
    nonReentrant
    whenNotPaused
{
    address account = _msgSender();
    ActiveQuest storage activeQuest = _activeQuests[activeQuestId];

    // Make sure active quest exists
    require(
        activeQuest.status == ActiveQuestStatus.IN_PROGRESS,
        "INVALID_ACTIVE_QUEST_ID: ActiveQuest is not IN_PROGRESS"
    );

    // Check to make sure sender is the quest owner
    require(
        activeQuest.account == account,
        "INVALID_ACCOUNT: Sender did not undertake this quest"
    );

    // Make sure quest is still active
    QuestDefinition storage questDef = _questDefinitions[
        activeQuest.questId
    ];
    require(
        questDef.active == true,
        "QUEST_NOT_ACTIVE: Cannot complete inactive quest"
    );

    uint256 endTime = activeQuest.startTime + questDef.completionSeconds;
    require(
        endTime <= block.timestamp,
        "NOT_READY_TO_COMPLETE: Quest is not ready to be completed"
    );

    // TODO: Maybe we fail here automatically if expire time has passed instead of error?
    require(
        questDef.expireSeconds == 0 ||
            (endTime + questDef.expireSeconds > block.timestamp),
        "QUEST_HAS_EXPIRED: Quest has expired and is no longer completeable"
    );

    // Figure out final amount of gold the player earns with some randomness
    uint256 requestId = _requestRandomWords(1);
    _vrfRequests[requestId] = VRFRequest({
        account: account,
        activeQuestId: activeQuestId
    });

    // Change status
    activeQuest.status = ActiveQuestStatus.GENERATING_RESULTS;
}

\color{black} \color{white}

Considering that the accountData.completions[questId] mapping is only updated after a quest is completed successfully, the QuestDefinition.MaxCompletions value can be easily bypassed.

As explained in the QUESTDEFINITION.MAXCOMPLETIONS CAN BE BYPASSED BY STARTING THE SAME QUEST MULTIPLE TIMES BEFORE COMPLETING THEM issue, a user can start a quest as many times as he wants as long as he has enough inputs.

For ERC721 and ERC1155 inputs, as these assets are locked once a quest is started, it is not possible to perform the same quest twice. But for ERC20 inputs as there is an open TODO and this is not implemented yet, the ERC20 tokens are not locked when the quest is started, any user can start the same quest as many times as he wants.

Then, after waiting a certain period of time, the same user could complete all the quests that he started. Each completion would make use of Chainlink VRF subscription. It would be possible to totally drain all the LINK balance of the subscription, as there is no limit on how many times the user could start the same quest.

RISK ACCEPTED: No mitigation was added to prevent this issue.

In the CraftingSystem contract, the craft() function is called every time a new craft is started:

CraftingSystem.sol

} else if (tokenType == GameRegistryLibrary.TokenType.ERC721) {
            // Auto-Lock NFT if necessary
            ILockingSystem lockingSystem = _lockingSystem();
            if (
                lockingSystem.isNFTLocked(
                    input.tokenContract,
                    input.tokenId
                ) == false
            ) {
                lockingSystem.lockNFT(input.tokenContract, input.tokenId);
            }

\color{black} \color{white}

As we can see, when ERC721 tokens are used as inputs, the function does not check that the token is owned by the caller. Moreover, when an NFT is used as an input for a craft, they are locked and an exclusive reservation is created. Once the craft is completed, the exclusive reservation is removed, but not the lock.

Similar to what was described in HAL-01 issue, initially a user would not be able to use the NFT of another user as input as during the lockNFT() call the transaction would revert with the ORIGIN_NOT_NFT_OWNER error.

Although, once the original owner has started and completed that craft with that NFT as input, the NFT would remain locked and as this NFT is already locked any user would be able now to start a craft using that NFT as the NFT ownership is not checked to create a reservation.

SOLVED: The Proof of Play team added the GameHelperLibrary._verifyInputOwnership(input, account); call in the craft() function that validates that the inputs are owned by the caller.

In the CraftingSystem contract, the craft() function can be called passing a 0 value as a craftAmount. It is not possible to exploit this in any way when ERC1155 tokens are used as inputs, as these errors would stop the exploit: RESERVE_AMOUNT_MUST_BE_NON_ZERO, UNLOCK_AMOUNT_MUST_BE_NON_ZERO.

When using ERC721 tokens as inputs, as they get an exclusive reservation, this value is not even used, and it is not possible to abuse this.

But when using ERC20 tokens as inputs, it is possible to call this craft() function infinite times with no cost as no ERC20 tokens would be burnt because inputDef.tokenPointer.amount * params.craftAmount would always be zero.

The attacker will never receive any reward as _completeRecipe will always be called with params.craftAmount = 0 but with every craft() call, if the RecipeDefinition.needsVRF == True, a Chainlink VRF request will be done. This means that any malicious user could abuse this in order to drain the Chainlink VRF subscription.

SOLVED: The Proof of Play team added a require statement in the craft() function that enforces that craftAmount is higher than zero.

In the CraftingSystem contract, when a craft is completed, the lastCompletionTime in the _accountData mapping is not updated. This means that the cooldown value is never considered, so all crafts have no cooldown.

PARTIALLY SOLVED: The Proof of Play team now updates the lastCompletionTime value every time _completeRecipe() is called. Although, this value will be updated even if no crafts are completed successfully. This value should only be updated if at least one craft was completed successfully.

In the QuestSystem contract, the _isQuestAvailable() function is called every time a new quest is started:

QuestSystem.sol

function _isQuestAvailable(
    address account,
    uint32 questId,
    QuestDefinition memory questDef
) internal view returns (bool) {
    if (!questDef.active) {
        return false;
    }

    // Perform all requirement checks
    IRequirementSystem requirementSystem = IRequirementSystem(
        _getSystem(GameRegistryLibrary.REQUIREMENT_SYSTEM)
    );
    if (
        requirementSystem.performAccountCheckBatch(
            account,
            questDef.requirements
        ) == false
    ) {
        return false;
    }

    // Make sure user hasn't completed already
    AccountData storage accountData = _accountData[account];
    if (
        questDef.maxCompletions > 0 &&
        accountData.completions[questId] >= questDef.maxCompletions
    ) {
        return false;
    }

    // Make sure enough time has passed before completions
    if (questDef.cooldownSeconds > 0) {
        if (
            accountData.lastCompletionTime[questId] +
                questDef.cooldownSeconds >
            block.timestamp
        ) {
            return false;
        }
    }

    return true;
}

\color{black} \color{white}

As we can see, one of the requirements checked is that the quest has not already been completed more than the QuestDefinition.MaxCompletions set. Although, the accountData.completions[questId] is only updated when a quest is completed successfully.

This means that a user who has enough inputs can bypass this QuestDefinition.MaxCompletions limit and complete the quests as many times as the inputs he owns allows him. All he has to do, is starting the same questId consecutively, as many times as his inputs allows him, before completing them.

RISK ACCEPTED: The Proof of Play team accepted the risk of this finding.

The LootSystem contract is importing the PausableUpgradeable library, although it is not making use of the whenNotPaused modifier anywhere in the code. Moreover, the contract is initialized in a paused state:

LootSystem.sol

function initialize(address gameRegistryAddress) public initializer {
    __Pausable_init();
    __ReentrancyGuard_init();
    __GameRegistryConsumer_init(gameRegistryAddress);
    _pause();
    _nullLoot = Loot({
        lootType: LootType.UNDEFINED,
        amount: 0,
        tokenContract: address(0),
        lootId: 0
    });
}

\color{black} \color{white}

All the core public/external functions in the LootSystem contract are missing the whenNotPaused modifier. With this modifier, functionality could be temporarily paused by the owner of the contract in case of having any issue in the smart contracts.

RISK ACCEPTED: The Proof of Play team accepted the risk of this finding.

The LootSystem contract makes use of the IGameNFTLoot.mintBatch() function to assign LootType.ERC721:

LootSystem.sol

function _mintLoot(address to, Loot memory loot) private {
    if (loot.lootType == LootType.ERC20) {
        IGameCurrency(loot.tokenContract).mint(to, loot.amount);
    } else if (loot.lootType == LootType.ERC1155) {
        IGameItems(loot.tokenContract).mint(
            to,
            SafeCast.toUint32(loot.lootId),
            loot.amount,
            true
        );
    } else if (loot.lootType == LootType.ERC721) {
        IGameNFTLoot(loot.tokenContract).mintBatch(to, 1);
    } else if (loot.lootType == LootType.UNDEFINED) {
        // Do nothing, NOOP
        return;
    } else {
        require(false, "INVALID_LOOT_TYPE: Invalid loot type for mintLoot");
    }
}

\color{black} \color{white}

Although this mintBatch() function is not implemented anywhere in the code, not even in the GameNFT contract:

RISK ACCEPTED: The Proof of Play team accepted the risk of this finding. Currently, the mintBatch() function is not implemented in the GameNFT contract.

The GameGlobals contract allows setting global variables that can be used by any other system/contract. This contract takes an ID and sets a value, that can be a boolean, string, uint, int or arrays of those types.

Although as the following functions contains a wrong require statement, the view function will never return any result as they will revert every time:

GameGlobals.sol

function getUint256(uint32 globalId)
    external
    view
    override
    returns (uint256)
{
    GlobalDataType dataType = _globalMetadata[globalId].dataType;
    require(
        dataType != GlobalDataType.UINT &&
            dataType != GlobalDataType.NOT_INITIALIZED,
        "GLOBAL_NOT_UINT256: Global is not initialized to a integer type"
    );

    return _globalValueUint256[globalId];
}

\color{black} \color{white}

dataType != GlobalDataType.UINT should be dataType == GlobalDataType.UINT

GameGlobals.sol

function getInt256(uint32 globalId)
    external
    view
    override
    returns (int256)
{
    GlobalDataType dataType = _globalMetadata[globalId].dataType;
    require(
        dataType != GlobalDataType.INT &&
            dataType != GlobalDataType.NOT_INITIALIZED,
        "GLOBAL_NOT_UINT256: Global is not initialized to a integer type"
    );

    return _globalValueInt256[globalId];
}

\color{black} \color{white}

dataType != GlobalDataType.INT should be dataType == GlobalDataType.INT

SOLVED: The Proof of Play team corrected the require statements mentioned in the GameGlobals contract.

In the QuestSystem contract, the following structure is defined:

QuestSystem.sol

struct QuestInput {
    // Pointer to a token (if ERC20, ERC721, or ERC1155 input type)
    GameRegistryLibrary.TokenPointer tokenPointer;
    // Traits to check against
    TraitsLibrary.TraitCheck[] traitChecks;
    // Whether or not this input is required
    bool required;
    // Whether or not the input is burned
    bool consumable;
    // Chance of losing the consumable item on a failure, 0 - 10000 (0 = 0%, 10000 = 100%)
    uint32 failureBurnRate;
    // Chance of burning the consumable item on success, 0 - 10000 (0 = 0%, 10000 = 100%)
    uint32 successBurnRate;
    // Amount of XP gained by this input (ERC721-types only, 0 - 10000 (0 = 0%, 10000 = 100%))
    uint32 xpEarnedPercent;
}

\color{black} \color{white}

This structure is used to define the items that a user should have to perform a specific quest. One of the fields of that struct is the required field, which apparently should be used by the smart contract to determine whether the input is required to start a quest or not.

In the case that the required field is false the quest should not force the user to have that input to start the quest although the required field is not checked anywhere in the QuestSystem smart contract and the contract always enforces the user to have that input.

RISK ACCEPTED: The Proof of Play team accepted the risk of this finding.

Multiple contracts are using the Initializable module from OpenZeppelin. To prevent leaving an implementation contract uninitialized OpenZeppelin's documentation recommends adding the _disableInitializers function in the constructor to automatically lock the contracts when they are deployed:

DisableInitializers function

/**
 * @dev Locks the contract, preventing any future reinitialization. This cannot be part of an initializer call.
 * Calling this in the constructor of a contract will prevent that contract from being initialized or reinitialized
 * to any version. It is recommended to use this to lock implementation contracts that are designed to be called
 * through proxies.
 */
function _disableInitializers() internal virtual {
    require(!_initializing, "Initializable: contract is initializing");
    if (_initialized < type(uint8).max) {
        _initialized = type(uint8).max;
        emit Initialized(type(uint8).max);
    }
}

\color{black} \color{white}

RISK ACCEPTED: The Proof of Play team accepted the risk of this finding.

In the LockingSystem contract, the rescueUnlockNFT() and rescueUnlockItems() are used in case of an emergency, so users can bypass all reservations and unlock their NFTs and items:

LockingSystem.sol

/**
 * @notice Bypasses all reservations and lets the user forcibly unlock their NFT
 *
 * @param tokenContract  Token Contract to unlock
 * @param tokenId        Token Id to unlock
 */
function rescueUnlockNFT(address tokenContract, uint256 tokenId)
    external
    nonReentrant
{
    require(
        rescueUnlockEnabled == true,
        "RESCUE_NOT_ENABLED: Rescue mode not enabled"
    );
    require(
        IGameNFT(tokenContract).ownerOf(tokenId) == _msgSender(),
        "SENDER_NOT_NFT_OWNER: sender must be token owner"
    );

    // Delete lock
    delete _lockedNFTs[tokenContract][tokenId];

    // Unlock token, if locked
    if (IGameNFT(tokenContract).isLocked(tokenId)) {
        IGameNFT(tokenContract).unlockToken(tokenId);
    }
}

/**
 * @notice Bypasses all reservations and lets the user forcibly unlock their items.
 *
 * @param account        Account to unlock items for
 * @param tokenContract  Token Contract to unlock
 * @param tokenId        Token Id to unlock
 */
function rescueUnlockItems(
    address account,
    address tokenContract,
    uint256 tokenId
) external nonReentrant {
    require(
        rescueUnlockEnabled == true,
        "RESCUE_NOT_ENABLED: Rescue mode not enabled"
    );
    require(
        account == _msgSender(),
        "SENDER_NOT_ITEM_OWNER: sender must be token owner"
    );

    // Delete any lock data
    delete _lockedItems[account][tokenContract][tokenId];

    // Unlock token, if locked
    uint256 balanceLocked = IGameItems(tokenContract).amountLocked(
        account,
        tokenId
    );
    if (balanceLocked > 0) {
        IGameItems(tokenContract).unlockToken(
            account,
            tokenId,
            balanceLocked
        );
    }
}

\color{black} \color{white}

In case that one of those NFTs rescued are staked in the StakingSystem contract, any call to claimGameNFTStakeRewards() or claimNavyStakeRewards() trying to unstake would revert. The claimGameNFTStakeRewards() function would call removeNFTReservation() causing an underflow in the following line:

LockingSystem.sol

function removeNFTReservation(
    address tokenContract,
    uint256 tokenId,
    uint32 reservationId
) external override onlyRole(GameRegistryLibrary.GAME_LOGIC_CONTRACT_ROLE) {
    NFTLockStatus storage lockStatus = _lockedNFTs[tokenContract][tokenId];
    NFTReservation storage reservation = lockStatus.reservations[
        reservationId
    ];

    // Make sure reservation exists
    require(
        reservation.timestamp > 0,
        "RESERVATION_NOT_FOUND: No reservation was found with that id"
    );

    // Remove from Ids array
    uint32 index = lockStatus.reservationIndexes[reservationId];
    if (index != lockStatus.reservationIds.length - 1) {
        uint32 lastId = lockStatus.reservationIds[
            lockStatus.reservationIds.length - 1
        ];
        lockStatus.reservationIds[index] = lastId;
        lockStatus.reservationIndexes[lastId] = index;
    }

    // Remove from all array
    lockStatus.reservationIds.pop();

    // Update the reserved amount if it was an exclusive reservation
    if (reservation.exclusive) {
        lockStatus.hasExclusiveReservation = false;
    }

    // Delete the reservation mapping
    delete lockStatus.reservations[reservationId];

    // Delete index mapping
    delete lockStatus.reservationIndexes[reservationId];
}

\color{black} \color{white}

In the case of claimNavyStakeRewards(), removeItemReservation() would be called, and again, an underflow would occur in the following line:

LockingSystem.sol

function removeItemReservation(
    address account,
    address tokenContract,
    uint256 tokenId,
    uint32 reservationId
) external override onlyRole(GameRegistryLibrary.GAME_LOGIC_CONTRACT_ROLE) {
    ItemLockStatus storage lockStatus = _lockedItems[account][
        tokenContract
    ][tokenId];
    ItemReservation storage reservation = lockStatus.reservations[
        reservationId
    ];

    // Make sure reservation exists
    require(
        reservation.timestamp > 0,
        "RESERVATION_NOT_FOUND: No reservation was found with that id"
    );

    // Remove from Ids array
    uint32 index = lockStatus.reservationIndexes[reservationId];
    if (index != lockStatus.reservationIds.length - 1) {
        uint32 lastId = lockStatus.reservationIds[
            lockStatus.reservationIds.length - 1
        ];
        lockStatus.reservationIds[index] = lastId;
        lockStatus.reservationIndexes[lastId] = index;
    }

    // Remove from all array
    lockStatus.reservationIds.pop();

    // Update the reserved amount if it was an exclusive reservation
    if (reservation.exclusive) {
        lockStatus.amountExclusivelyReserved -= reservation.amount;
    } else {
        // Calculate number of items needed for non-exclusive reservation
        uint256 max = 0;
        for (
            uint256 idx = 0;
            idx < lockStatus.reservationIds.length;
            idx++
        ) {
            ItemReservation storage otherReservation = lockStatus
                .reservations[lockStatus.reservationIds[idx]];
            if (
                otherReservation.exclusive == false &&
                otherReservation.amount > max
            ) {
                max = otherReservation.amount;
            }
        }

        lockStatus.maxNonExclusiveReserved = max;
    }

    // Delete the reservation mapping
    delete lockStatus.reservations[reservationId];

    // Delete index mapping
    delete lockStatus.reservationIndexes[reservationId];
}

\color{black} \color{white}

ACKNOWLEDGED: The Proof of Play team acknowledged this finding. The team claims that if LockingSystem.setRescueUnlockEnabled(True) is called, they are abandoning the staking contract. It is the "nuclear" option to make sure player assets are always recoverable.

tx.origin-based protection can be abused by a malicious contract if a legitimate user interacts with the malicious contract. For example:

Example.sol

contract TxOrigin {
    address owner = msg.sender;

    function bug() {
        require(tx.origin == owner);
    }

Bob owns TxOrigin. Bob calls Eve's contract. Eve's contract calls TxOrigin and bypasses the tx.origin protection.

Code Location

ERC721Lockable.sol

• Line 36: ownerOf(tokenId) == tx.origin,
• Line 71: tx.origin == ownerOf(tokenId),
• Line 91: tx.origin == ownerOf(tokenId),

GameItems.sol

• Line 185: tx.origin == account,
• Line 210: tx.origin == account,

GameNFT.sol

• Line 66: tx.origin == ownerOf(tokenId),
• Line 85: tx.origin == ownerOf(tokenId),

StakingSystem.sol

• Line 215: tx.origin == _msgSender() ||
• Line 224: account == tx.origin,
• Line 293: tx.origin == _msgSender() ||
• Line 302: account == tx.origin,
• Line 396: tx.origin == _msgSender(),
• Line 431: tx.origin == account,
• Line 661: address relevantAccount = tx.origin;

PirateGameV1.sol

• Line 188: tx.origin == _msgSender(),
• Line 272: NFT storage captainNFT = captainNFTs[tx.origin];
• Line 293: IGameNFT(tokenContract).ownerOf(tokenId) == tx.origin,
• Line 352: IGameNFT(nftContract).ownerOf(nftTokenId) == Line
• Line 380: goldToken.burn(tx.origin, goldRequired);

ACKNOWLEDGED: The Proof of Play team acknowledged this finding.

State variables can be declared as constant or immutable. In both cases, the variables cannot be modified after the contract has been constructed. For constant variables, the value has to be fixed at compile-time, while for immutable, it can still be assigned at construction time. The following state variables are missing the constant modifier:

Randomizer.sol

• Line 35: uint32 callbackGasLimit = 1000000;
• Line 38: uint16 requestConfirmations = 3;

SOLVED: The Proof of Play team declared the state variables mentioned as constant.

As i is an uint256, it is already initialized to 0. uint256 i = 0 reassigns the 0 to i which wastes gas.

Code Location

TraitsConsumer.sol

• Line 437: for (uint32 i = 0; i < traitIds.length; i++) {

GameItems.sol

• Line 339: for (uint8 idx = 0; idx < ids.length; idx++) {

GameNFT.sol

• Line 178: for (uint32 i = 0; i < traitIds.length; i++) {

StakingSystem.sol

• Line 235: for (uint256 i = 0; i < tokenIds.length; i++) {
• Line 325: for (uint256 i = 0; i < tokenIds.length; i++) {
• Line 410: for (uint256 idx = 0; idx < nftContracts.length; idx++) {
• Line 439: for (uint256 idx = 0; idx < stakeIndexes.length; idx++) {
• Line 546: for (uint256 idx = 0; idx < nftStake.gameItemStakes.length; idx++) {
• Line 678: for (uint256 i = 0; i < nftStake.gameItemStakes.length; i++) {

PirateGameV1.sol

• Line 228: for (uint32 i = 0; i < tokenIds.length; i++) {
• Line 423: for (uint8 i = 0; i < shipBondingSupplyPercent.length; i++) {
• Line 486: for (uint256 i = 0; i < amount; i++) {

RaffleMintV1.sol

• Line 313: for (uint256 i = 0; i < tickets.length; i++) {
• Line 553: for (uint256 i = 0; i < addresses.length; i++) {
• Line 672: for (uint256 i = 0; i < amount; i++) {
• Line 702: for (uint256 i = 0; i < amount; i++) {

ERC1155Lockable.sol

• Line 185: for (uint8 idx = 0; idx < ids.length; idx++) {

LockingSystem.sol

• Line 198: for (uint256 idx = 0; idx < tokenIds.length; idx++) {
• Line 228: for (uint256 idx = 0; idx < tokenIds.length; idx++) {
• Line 463: for (uint256 idx = 0; idx < lockStatus.reservationIds.length; idx++)

TraitsProvider.sol

• Line 128: for (uint256 idx = 0; idx < traitIds.length; idx++) {
• Line 174: for (uint256 idx = 0; idx < traitIds.length; idx++) {

SOLVED: The Proof of Play team followed Halborn's suggestion and does not initialize uint variables to 0 reducing the gas costs.

In the loops below, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++.

Code Location

TraitsConsumer.sol

• Line 437: for (uint32 i = 0; i < traitIds.length; i++) {

GameItems.sol

• Line 339: for (uint8 idx = 0; idx < ids.length; idx++) {

GameNFT.sol

• Line 178: for (uint32 i = 0; i < traitIds.length; i++) {

StakingSystem.sol

• Line 235: for (uint256 i = 0; i < tokenIds.length; i++) {
• Line 325: for (uint256 i = 0; i < tokenIds.length; i++) {
• Line 410: for (uint256 idx = 0; idx < nftContracts.length; idx++) {
• Line 439: for (uint256 idx = 0; idx < stakeIndexes.length; idx++) {
• Line 546: for (uint256 idx = 0; idx < nftStake.gameItemStakes.length; idx++) {
• Line 678: for (uint256 i = 0; i < nftStake.gameItemStakes.length; i++) {

ERC721BridgableChild.sol

• Line 51: for (uint256 i; i < length; i++) {
• Line 150: for (uint256 i; i < length; i++) {

PirateGameV1.sol

• Line 228: for (uint32 i = 0; i < tokenIds.length; i++) {
• Line 423: for (uint8 i = 0; i < shipBondingSupplyPercent.length; i++) {
• Line 486: for (uint256 i = 0; i < amount; i++) {

RaffleMintV1.sol

• Line 313: for (uint256 i = 0; i < tickets.length; i++) {
• Line 553: for (uint256 i = 0; i < addresses.length; i++) {
• Line 672: for (uint256 i = 0; i < amount; i++) {
• Line 702: for (uint256 i = 0; i < amount; i++) {

ERC1155Lockable.sol

• Line 185: for (uint8 idx = 0; idx < ids.length; idx++) {

LockingSystem.sol

• Line 198: for (uint256 idx = 0; idx < tokenIds.length; idx++) {
• Line 228: for (uint256 idx = 0; idx < tokenIds.length; idx++) {
• Line 463: for (uint256 idx = 0; idx < lockStatus.reservationIds.length; idx++)

TraitsProvider.sol

• Line 128: for (uint256 idx = 0; idx < traitIds.length; idx++) {
• Line 174: for (uint256 idx = 0; idx < traitIds.length; idx++) {

For example, based in the following test contract:

Test.sol

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

contract test {
    function postiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; i++) {
        }
    }
    function preiincrement(uint256 iterations) public {
        for (uint256 i = 0; i < iterations; ++i) {
        }
    }
}

We can see the difference in the gas costs:

SOLVED: The Proof of Play team followed Halborn's suggestion and uses now ++i instead of i++ to increment the value of uint variables inside loops.

In the PirateGameV1 contract, the function _finishMintShips() contains the following code:

PirateGameV1.sol

function _finishMintShips(
    address to,
    uint32 amount,
    bool lock,
    uint256 randomness
) private {
    uint256 numPirateShips = 0;
    uint256 numNavyShips = 0;

    uint256 seed = 0;
    for (uint256 i = 0; i < amount; i++) {
        // Pick pirate or navy
        seed = uint256(keccak256(abi.encode(randomness, i)));

        // 20% chance of navy
        if ((seed & 0xFFFF) % 5 == 0) {
            numNavyShips++;
        } else {
            numPirateShips++;
        }
    }

    // Subtract pending mints
    mintsPending -= amount;

    uint256[] memory typeIds = new uint256[](2);
    uint256[] memory balances = new uint256[](2);

    if (numPirateShips > 0) {
        gameItems.mint(to, pirateShipTypeId, numPirateShips, lock);
        typeIds[0] = pirateShipTypeId;
        balances[0] = numPirateShips;
    }

    if (numNavyShips > 0) {
        gameItems.mint(to, navyShipTypeId, numNavyShips, lock);
        typeIds[1] = navyShipTypeId;
        balances[1] = numNavyShips;
    }
}

\color{black} \color{white} As we can see above, the arrays typeIds and balances are created in memory but they are not used anywhere in the code; hence they can be removed to reduce the gas costs.

SOLVED: The Proof of Play team solved this issue.

In the RaffleMintV1 contract, there is no view function that displays what tickets are currently owned by a user.

The tickets would be mixed after clearRaffle() is called, which makes it very difficult for users to know which tickets they own. To get that information, they would have to manually query every single position of the raffleEntries array.

A view function, that displays the tickets owned by the user, would be especially useful to properly do the claimRaffle() call.

SOLVED: The Proof of Play team corrected this issue. The function getEntriesForAddress() can be used to display what tickets are currently owned by the caller.

In the StakingSystem contract, in the fulfillRandomWordsCallback() function above the coinFlips state variable declaration, there is a comment that states:

"This should not overflow since the balance is determined by gameplay logic and the user will not have more than 256 ships per stake."

This comment is incorrect, as a 2^256 would definitely overflow. In this case, the maximum amount of ships per stake is 50 which corresponds to a Pirate with the maximum Command Rank:

StakingSystem.sol

// Number of ships a pirate can command for each command rank
uint8[] public SHIPS_PER_COMMAND_RANK = [
    0,
    5,
    10,
    15,
    20,
    25,
    30,
    35,
    40,
    45,
    50
];

ACKNOWLEDGED: The Proof of Play team acknowledged this finding.