Abraxas MintHub

Description

The number of users that can enter the raffle is intentionally limited to MAX_PLATFORM_USERS to make the processing feasible. DoS on the supported user slots is prevented using a combination of minimumBalance requirement and the ability for the admin to forcefully remove the users using the returnDeposits(...) function. But given that returnDeposits(...) uses _performTransfer(...) to send the user's balance, a user can revert that call and disallow the admin from forcefully removing them. This allows griefing users to squat in the supported user slots (without participating in any/many raffles) and prevent other legitimate users from participating in the raffles.

function _performTransfer(address recipient_, uint256 value_) internal {    (bool succeeded, ) = recipient_.call{ value: value_ }("");    if (!succeeded) {      revert("Transfer failed");    }  }

Recommendation

Consider wrapping the native token and sending wrapped tokens in case of failed native token transfer. Also, limit the amount of gas that is passed during such a transfer.

Description

PLATFORM_ADMIN_ROLE is allowed to call updateRaffle(...) in any stage of the raffle to update aspects related to ticketPrice, closedTimestamp, nftAddress, tokenIds, projectTreasury and raffleFeePercentage. This supposedly is intended to mainly allow the project to correct any wrong list of tokenIds provided earlier, which may happen because NFT ownership is not validated earlier.

However, this privileged action by PLATFORM_ADMIN_ROLE may cause unexpected behavior for users who may not observe such updates in time and therefore end up participating in raffles with updated parameters they did not accept earlier. This also allows a big window of opportunity for abuse by the platform, especially in stages beyond PreDraw.

Possible scenario:

1. Alice notices an upcoming raffle for a NFT collection she likes.
2. Alice deposits sufficient funds to participate in this raffle.
3. Platform later updates the NFT collection to a different one after raffle starts.
4. Alice fails to notice the update and so is considered as a participant in the updated raffle for NFTs she does not like.
5. Alice is one of the winners and pays the ticket price for an NFT token she did not mean to receive during sign up.

Recommendation

Consider restricting updateRaffle(...) to only PreDraw stage so that all raffle parameters are locked beyond it, which retains a window to fix any errors but reduces the window for accidents or abuse.

Description

The platform implements constant MAX_PLATFORM_USERS = 10_000_000, which is meant to specify the maximum number of simultaneous users who can make use of this platform. And this limit allows the use of a fixed-length array for ticketRanges, thereby reducing storage costs.

However, the implemented check in depositForUser(...) is:

if (newKey && userBalances.length() > (MAX_PLATFORM_USERS - 1)) {     revert("Max platform users reached");}

where the -1 in bounds check reverts when we are still one less than MAX_PLATFORM_USERS instead of when we have reached it. Therefore, the intended maximum platform user limit is never reached.

Recommendation

Consider fixing the check, for example, by removing -1.

Description

Minthub platform uses Pyth Network as the entropy source for random number seed generation used in raffles. It treats zero as the default value of raffle.entropy.randomNumber assuming that zero is an invalid entropy result from Pyth Network. It enforces this assumption in entropyCallback(...) by checking raffle.entropy.randomNumber == bytes32(0) to detect any repeated callbacks. It again enforces this in drawWinningTicketsBatch(...) by checking against 0 to ensure that it has indeed received randomness.

However, while the probability of Pyth Network returning 0 as the random number is extremely small, it is still a valid result. Treating it as invalid will allow repeated callbacks and also prevent such a raffle from completing because of the enforced checks.

Recommendation

Reconsider treating zero as an invalid entropy returned from Pyth Network by making appropriate changes to related logic.

Description

Once the draw is complete i.e. the prizes are awarded, winners should be able to claim their reward even if the draw is cancelled for some reason by the PLATFORM_ADMIN. But currently, this is not possible because _distributePrize(...) requires the raffle state to be DrawComplete.

function _distributePrize(    Raffle storage raffle_,    uint256 raffleId_,    address nftContract_,    uint256 prizeIndex_  ) internal returns (bool transfered_) {    PrizeItem storage prize = raffle_.prizes.items[prizeIndex_];
    // We can't run this before we are at status DrawComplete.    if (raffle_.status != RaffleStatus.DrawComplete) {      revert("Draw is not complete");    }

Also, the state of ticket-fee-paid but prize-not-distributed is possible in the current implementation. This is unfair for the users when raffles are cancelled in such a state because they won't be able to receive their prize NFTs for which they already had paid the fee.

Recommendation

Consider allowing distribution of prizes for raffles that are cancelled after draw completion, or alternatively preventing cancellations after draw completion.

Description

Minthub supports a depositForUser(...) function to allow deposits for other users for allowing functionality such as promotions where the user gets a balance in the raffle that is funded by promoters/donations. However, this functionality can be abused by griefers to force participate users in raffles leading to unexpected loss of their balances. For example, anyone can donate to a user who is below minimumBalance (and expects to simply withdraw it) and has ParticipationListType == ALL, thereby forcing their participation in upcoming raffles where ticket prices >= balance.

Example scenario:

1. Alice has a balance of 49 MON on Minthub, where minimumBalance has been set to 50. She does not intent to participate in any future raffles but plans to withdraw her remaining balance.
2. Mallory notices an upcoming raffle where the ticket price is 50 MON and uses depositForUser(...) to deposit 1 MON for Alice.
3. Alice's balance is now 50 MON and she is forced to participate in the upcoming raffle given that her balance buys her 1 ticket.
4. If Alice is one of the raffle winners, she is deducted 50 MON and awarded the NFT.
5. Alice is now left with an NFT from a raffle she did not intend to participate and loses her 49 MON balance she intended to withdraw.

Recommendation

Consider weighing the need for and access to this griefing vector against the benefits of the supported functionality.

Description

Minthub uses Pyth Network to seed entropy for the random number generation for raffles. To obtain this service, the protocol needs to pay fees to Pyth Network, which differs for every chain and also varies over time depending on the chain's current gas price.

Pyth's documented recommendation is to use its on-chain method getFeeV2(...) to compute the required fee and send that amount as the value of the requestV2(...) call. However, Minthub's current design expects an offchain call to getFeeV2(...) and providing that as msg.value to startDraw(...) for its call to requestV2(...). While this may work under stable chain/fee conditions, the BATCH_OPERATOR_ROLE may have to overpay fees for startDraw(...) calls during any periods of volatility. Any underpayment will cause a revert (which on Monad uses up the entire gas sent with the transaction and requires resending it with correct fees) and overpayments will not be refunded, which leads to loss of protocol operational funds used for gas and fees.

Recommendation

Consider implementing Pyth's recommendation to use its on-chain method getFeeV2(...) to compute the required fee and refunding any excess msg.value sent by BATCH_OPERATOR_ROLE.

Description

Creating/updating of raffles checks if the NFT Treasury has approved Minthub to transfer NFTs. However, there are no ownership/existence checks for raffle NFTs. This supposedly is intentional to support flows where raffles are launched before the NFTs are in the NFT Treasury or before NFTs even exist.

While this is reasonable during raffle creation, the missing ownership/existence checks for raffle NFTs even in the later raffle stages may lead to NFT distribution failures if NFTs still do not exist or the NFT Treasury does not have their ownership. This would cause unexpected behavior for winning users who will not receive the previously advertised NFTs.

Recommendation

Consider adding ownership/existence checks for raffle NFTs before the DrawStarted stage to ensure that NFT Treasury owns all the listed NFTs.

Description

The prizeIndex_ parameter used in event TicketPaymentProcessed emitted in _processPayment(...) is derived from prize.prizeIndex. However, prize.prizeIndex stores an incorrect iteration index when prizes are added to the prizes array in _awardPrize(...) for batched calls to drawWinningTicketsBatch(...). This is because the starting index is ignored for successive batched calls.

If these events are consumed by offchain agents then they will associate an incorrect NFT prize with the winning tickets and addresses.

Recommendation

Consider using raffle.prizes.items.length instead for prizeIndex when pushing PrizeItem into raffle.prizes.items array, which is also the index used to determine the NFT tokenID to be distributed.

Description

ticketNumber and prizeIndex parameters of PrizeAwarded event have their positions incorrectly exchanged in the emit.

event PrizeAwarded(    uint32 indexed raffleId,    address indexed winner,    uint32 prizeIndex,    uint32 ticketNumber  );    .....  emit PrizeAwarded(uint32(raffleId_), winner_, ticketNumber_, prizeIndex_);

Recommendation

Change the ordering or change the event signature.

Description

Initializing variables to their default values is redundant and consumes valuable bytecode space and gas.

Recommendation

Consider removing these initializations wherever possible.

Description

Few of the privileged functions are missing event emissions. This prevents any offchain monitoring tools from observing these privileged actions and allowing users to react to them if necessary.

Recommendation

Consider emitting events for all privileged functions.

Description

enumerateTicketsBatch and drawWinningTicketsBatch rely on usersBalances being stable and not getting altered in between for its correct working. Although this requires unexpected behavior from privileged entities, such scenarios might accidentally happen.

Example scenarios include not obtaining the random number from Pyth, admin calling returnDeposits, admin not completing the various steps of the draw within the time limits etc. (eg: distributeAllPrizesForRaffle (...) of a raffle being called when another raffle is already under draw).

Recommendation

Ensure that the draw is cancelled if it cannot be fully completed before the allocated time limit.

Abraxas Minthub

Commit #4aba111.

Cantina

Across multiple commits, ending with Commit #4aba111, changes were made by adding logic for depositBalancesLastChangedTimestamp, raffle.stableBalancesRequiredFromTimestamp, onlyWhenStableBalances and onlyBatchOperatorWhenLocked to make the raffle process more robust against unexpected changes to user balances.

Description

distributeUserPrizes(...) iterates through the specific user's entire winningRafflesForUser and respective userPrizes arrays. Since elements are not removed from the array even after claiming, these arrays could grow to large lengths even under normal operations and could theoretically grow to enough elements to exhaust the block gas limit to perform the full iteration.

Recommendation

Consider adding a batch variant which limits the number of iterations per call.

Abraxas

Acknowledged.

Claims are always possible through more gas efficient methods, for example distributeSinglePrize. The method distributeUserPrizes(…) is provided solely as a convenience function for users with prizes across multiple raffles. Under expected processing all prizes will be distributed using distributeAllPrizesForRaffleBatch with no need to call distributeUserPrizes(…).

Cantina

Acknowledged.

Description

Even when users are not withdrawing themselves and deposits are being returned by the admin using the returnDeposits(...) function, the platform fee is still charged from users. And the admin can return deposits of any user irrespective of whether the user has > minimumBalance or not.

Recommendation

Consider not capturing the fee or disallow returnDeposits(...) for balances > minimumBalance.

Description

A defensive coding style is to add idempotent checks for when the new value is same as current value, especially for booleans. This allows timely detection of any inconsistency between onchain values and their offchain tracking.

Setters setDrawsPausedStatus(...) and setDepositsPausedStatus(...) are missing such idempotent checks.

Recommendation

Consider adding idempotent checks to setters.

Description

Deposit event parameters of caller and user addresses are missing the indexed attribute to allow faster lookups.

Recommendation

Consider adding the indexed attribute to Deposit event parameters.

Description

While updateRaffle(...) can be called in any raffle stage, LogRaffleStatus incorrectly logs the raffle status always as RaffleStatus.PreDraw. This will be incorrect if updateRaffle(...) is called in later stages.

Recommendation

Consider logging raffle.status instead of RaffleStatus.PreDraw to reflect the current raffle stage.

Description

The Natspec mentions the function name incorrectly as _lockOff instead of _unlock.

* @dev _lockOff Manually unlocks by setting the unlockAtTimestamp to block.timestamp.   */  function _unlock() internal {    unlockAtTimestamp = block.timestamp;  }

Recommendation

Change _lockOff to _unlock in the comment.

Description

Minthub has privileged roles of DEFAULT_ADMIN_ROLE, PLATFORM_ADMIN_ROLE, RAFFLE_ADMIN_ROLE and BATCH_OPERATOR_ROLE.

DEFAULT_ADMIN_ROLE can grant and revoke membership of other roles. PLATFORM_ADMIN_ROLE can set the minimum balance, NFT treasury address, platform fee treasury address, platform fee percentage, pause/unpause draws and deposits, cancel a raffle, unlock, update the details of a raffle, rescue ERC721 and ERC20 tokens and return all deposits to users. RAFFLE_ADMIN_ROLE can create raffles. BATCH_OPERATOR_ROLE can call batch operations that conduct the raffle draw, enumeration and distribution process.

Any compromised/malfunctional privileged role can cause unexpected behavior if it calls its authorized functions with incorrect values or in a delayed manner accidentally/intentionally. While there are implemented safeguards to mitigate many of these situations, this nevertheless poses a centralization risk.

Recommendation

Consider:

1. Implementing the highest levels of operational security for privileged role management.
2. Documenting and highlighting the assumptions and risks for platform users/owners.

Abraxas

Acknowledged.

We fully concur with these recommendations and have impressed upon the platform owner the need to implement stringent security for the privilege roles, in particular DEFAULT_ADMIN_ROLE and PLATFORM_ADMIN_ROLE. This recommendation was highlighted in our code handover and is prominently made in the Authority Model section of the repository readme.

Cantina

Acknowledged.

Description

There are many loops where the iteration conditional bound is always evaluated to determine the length. This can be cached to save gas.

Recommendation

Consider caching the loop iteration conditional bound length.

Description

The _gcd() function expects the bigger number to be passed in as the first argument. But currently, the smaller number candidate_ is passed as the first argument instead of N. This results in an unwanted additional iteration.

if (_gcd(candidate_, N) == 1) {

Recommendation

Pass N as the first argument.