Protocol Review, Part 1

Throughout the contracts there are myriad instances of:

• Lack of camelcase
• Unnecessary local variables which waste gas
• Redundant boolean checks e.g. == true
• Typos in the comments
• Unnecessary for-loops which waste gas and enable DoS
• Inefficient computations e.g. feeBalance -= feeBalance
• Many functions can be declared external

Use camelcase throughout the contracts, remove redundant and unnecessary local variables, do not perform redundant boolean checks, revise comments, refactor operations to avoid unnecessary for-loops, avoid inefficient computations e.g. use feeBalance = 0, declare all functions that are not called within the contracts external rather than public

Throughout the contracts there is a risk of admins and oracles using their privilege to benefit themselves or act malicious toward user’s holdings. Contracts utilizing the CustomAdmin access control model face more risk as the number of admins or oracles increase because it only takes one to act mischievous.

Additionally, the treasury which is under the in-house MultiSig.sol contract can be can be manipulated by the principal admin who can unethically force a quorum.

Ensure that privileged addresses such as admins are all a multi-sig and/or introduce a timelock for improved community oversight. Secure a KYC for increased community trust.

A malicious actor can call placeBet with different addresses, sending a tiny amount of ETH per call.

As a result, the YesBettors array and NoBettors array will expand to the point where it either exceeds the block gas limit, or costs too much to reportResult. This will render the function inoperable.

Use a pull-over-push withdrawal pattern such that the “for” loop can be avoided.

feeBettorBet is calculated based on total amount bet for a user i.e. winning side + losing side.

If the user bet more on the losing side, it is possible for the feeBettorBet to exceed the amount bet on the winning side, causing a subtraction underflow which will revert. Therefore, reportResult will consistently fail and users will not get their winnings.

Calculate the fee based on the user’s bet for the winning side. Or, place a cap on the fee.

The contract allows placing bets on both sides which makes sense if the user would like to perform arbitrage. If the event is canceled upon an emergency, a user can only withdraw funds from one side. If they were betting on both sides for arbitrage, they lose the amount they bet on the other side.

Don’t set both sides of a user’s bet to 0.

Place the require statements before the if statement in reportResult. It is not necessary to check those conditions upon each loop and also redundant to have them in loops for both sides while being extremely gas expensive.

Move the require statements to the top of the function.

The comment states that 1 represent YES and 0 represents NO. Because YES is the the first and default value of the enum, YES is 0 and 1 is NO.

Update the comment to accurately reflect the enum.

feePercentage and UltibetsTreasury are never mutated once set.

Add the “immutable” keyword to UltiBetsTreasury and add the “constant” keyword to feePercentage since it is not being set in the constructor..

There is no way to withdraw the Ether sent to the treasury. Contracts like SquidBetPrizePool send ether to the treasury when EmergencySafeWithdraw is called.

Add a function to convert the Ether to the fundingToken, or implement allocations to be able to use the Ether.

Functions deleteAllocation and changeSalary have no Admin requires so anyone can delete an allocation and change a team member’s salary.

Add a check that the msg.sender is an admin.

Admin can just keep calling createAllocation so the allocation for a particular address cannot be withdrawn as payoutday keeps getting pushed back.

Adopt a solution that doesn’t allow such manipulation, or ensure trust via a multi-sig for every privileged address.

The salary could be set arbitrarily high before someone calls withdraw to drain the fundingToken balance. The salary could also be set arbitrarily high to prevent withdrawal during totalpayout calculation by causing an overflow.

Add a cap to the salary and restrict access to changeSalary.

withdrawalFrequency can be set in the constructor. It doesn’t need to be updated each time an allocation is created. Especially since withdrawalFrequency is used in other functions like withdraw

Set the withdrawal frequency in the constructor.

Admin is not mutated outside of the constructor so it can be declared immutable.

Declare Admin with the immutable keyword.

The principalAdmin can keep calling setApproverAddr to add as many addresses as needed to reach quorum and approve any arbitrary transaction. This is not a true multi-sig if a principal admin can obtain all the power and decision making.

Make it so a majority of admins must agree to add or remove another admin or partake in other important decisions with the treasury.

There is no check to make sure that quorum is not set to 0.

Add a require that quorum is greater than 0.

If you found that the msg.sender is an admin you can break out of the for loop to save gas.

Break out of the for loop once the msg.sender is verified to be an admin

Because the transfer is from the current address to another address, the ERC20 function transfer could be used.

Replace the use of transferFrom with transfer. Be sure to check the return value or opt for a safeTransfer alternative.

The tokenId+1 is sent to the caller although it was not used in the check for a valid leaf. Instead, tokenId was verified to correspond with the msg.sender.

Send the current tokenId to msg.sender.

The comment states that the proof is to check that the address and the amount are in tree. However, for the ERC721 airdrop, you are checking if the address and tokenId are in the tree.

Update the comment to reflect the ERC721 NFT airdrop.

The comment states that the function adds the specified address to the list of administrators but it adds the address to the mapping of Oracles.

Update the comment to reflect what the function does.

In the placeBet function there is no check to ensure that isEventCancelled is false.

Add a require statement to all of these placeBet functions such that you cannot place a bet if the event is cancelled.

In the placeBet function the following assignment playerSide[msg.sender] += choice should simply read playerSide[msg.sender] = choice

Refactor this line to be playerSide[msg.sender] = choice.

Once the event is finished, it is possible to for a malicious oracle to call reportResult several times to add both sides as the winner.

Prevent the result from being changed by using a variable to check if the result was already reported.

In reportResult the for loop that loops through each bettor in the playersSide mapping is gas expensive and unnecessary.

Infer whether or not players are winners based on the result contract variable, don’t maintain the iswinner mapping.

Once the event is finished, it is possible to for a malicious oracle to repeatedly call reportResult to modify which side is the winner.

Prevent the result from being changed by using a variable to check if the result was already reported.

pickWinner uses weak sources of on-chain randomness. A validator can exploit this in order to obtain a winner that is beneficial to themselves.

In addition, an Admin can keep calling pickWinner, then reportResult to clear isCompetitionEnded, then call pickWinner again and so on until the winner is favorable to them.

Utilize a strong source of randomness whether it be the on-chain randomness pattern or an oracle. In addition, prevent repeated calls to pickWinner by tracking whether a winner was already chosen.

There is no need to cast a positive integer to a uint.

Remove the unnecessary uint surrounding 1 and 2.

The for loop over an arbitrary number of votes can be avoided by tallying the votes in the Vote function.

Get rid of the for loop and move on-demand tallying logic to Vote.

The playerVote state variable is never meaningfully used.

Remove the playerVote variable.

It is a waste of gas to initialize the nextPlayerNumber to 0 and then immediately increment it in the constructor.

Simply initialize nextPlayerNumber to 1 rather than initializing it to 0 and spending gas to increment it in the constructor.

The error message indicates that the cost is 0.01 Ether while it is in fact 1 Ether.

Update the message to reflect the real cost.

The playersNumbers mapping is updated as follows: playersNumbers[msg.sender] += nextPlayerNumber. It should instead be assigned like so playersNumbers[msg.sender] = nextPlayerNumber

Update the assignment to use = rather than +=.

The function getIsRegisterdPlayer contains a typo

Update the name to be getIsRegisteredPlayer.

The numberOfPlayersRegistered variable can simply be derived as one less than the nextplayerNumber and is therefore unnecessary.

Remove the numberOfPlayersRegistered variable and rely on the nextplayerNumber - 1.

Nothing prevents the UltiBetsTreasury from being set to the zero address in the constructor.

In addWinnerAddress, the winner address can be set to 0.

Add zero address checks using require statements.

If there are a large amount of winners, the amount of gas can exceed the block limit and winners will not be able to get the prize money,

Utilize a pull-over-push withdrawal patten.

There is not a need to have both winnerClaimPrizePool and winnersClaimEqualPrizePool. A single winner address could be stored as an “equal” winner and the prize money would just be sent to that single address.

Remove the need to store the winnerAddress and solely store winners in equalWinners.