Protocol Review, Part 2

Throughout the contracts there are myriad instances of:

• Lack of camelcase
• Use of SafeMath when Solidity version ^0.8.0 has overflow and underflow checks
• Unnecessary local variables which waste gas e.g. address to = payable(ultibetsTreasury)
• Redundant boolean checks e.g. == true
• Typos in the comments
• 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, remove SafeMath operations to save gas, declare functions not used internally as external.

• Typos, code-style, function declarations, and frivolous code has been addressed.

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.

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.

• The risk is acknowledged and will be limited with appropriate measures such as multi-sig addresses and community transparency.

The bets mapping is not being updated in placeBet. When withdrawGain is called, .div(bets[result.winner]) would lead to a division by 0 error every time. Therefore, no one would be able to withdraw their gains, leading to complete loss of funds.

In placeBet, increment the bets mapping with bets[_side] += betAmount.

• The bets mapping is now updated in placeBet.

The logic does not correctly calculate the gain due to the SafeMath order of operations. With the SafeMath operations the addition is performed first, but what is needed is to first calculate the winner’s part of the loser funds and then finally adding it to BettorBetWinner.

For example:

3.add(10).mul(3).div(6) = 6

3 + 10 * 3 / 6 = 8

Replace with BettorBetWinner + bets[result.loser] * BettorBetWinner / bets[result.winner].

• The logic has been replaced as suggested.

reportResult does not require the event to be finished. Therefore, the result can be reported multiple times and the winner and loser swapped while the treasury fee is taken multiple times.

Add require(!isEventFinished) in reportResult so a result cannot be reported twice.

• The suggested requires has been added.

reportResult does not prevent the oracle from setting the winner and loser to the same side which can lead to loss of funds for many users.

Consider a 100 ether bet on one side and 10 ether bet on another, but the 100 ether side is chosen as both the winner and loser. From the calculation in withdrawGain, the contract will attempt to payout a total of 200 ether while it only holds 110. Therefore the users who claim first will deplete the contract and the ones who claim later will experience a complete loss of funds.

Add a safety check that the arguments _winner and _loser are different.

• The suggested requires has been added.

reportResult depends on the ETH transfer to the treasury to be successful in order for the result to be reported. Such a call can fail, leading to a DoS condition.

Remove lines 185-187 as there is no need to have the transfer logic there. External calls should ideally be isolated into another transaction and the admin calling withdrawEarnedFees serves that purpose.

• Unnecessary transfer logic has been removed.

The comment for stopBet states that “... this action can only be performed by an administrator” but the modifier is OnlyOracle. In addition, the comment states that it is an emergency function, yet it is required to be called in order for a bettor to withdrawGain.

Update the comments to accurately reflect the function. In addition, verify if stopBet is needed. If it is truly only used for emergency situations, it does not make sense to have it required so user’s can withdraw their gains.

• The comment has been updated.

The betAmountForYes, and betAmountForNo variables are never assigned in the bettorBetHistory mapping.

Remove them from the struct or use them to replace the betsAmountPerBettor mapping.

• The variables have been removed.

The inArrayYes and inArrayNo variables are never used.

Remove them.

• The variables have been removed.

Because there is no receive or fallback function, any contract that relies on sending ETH to the treasury will face unintended consequences such as stuck fees.

Add a receive() external payable { } function to the contract.

• A receive function has been added.

The require(msg.sender == Admin) can be deduplicated into an onlyAdmin modifier used for every permissioned function.

Deduplicate the require logic into an onlyAdmin modifier and apply the onlyAdmin modifier to every permissioned function.

• The modifier has been created and added to relevant functions.

The _descreption parameter in createAllocation is misspelled.

Correct it to _description.

• The typo has been fixed.

Since the amount variable is only used once and the allocations[msg.sender].salary value is not changed before the use of amount, it is unnecessary to declare the amount variable.

Remove the declaration and use of the amount variable for gas optimization.

• The variable has been removed.

The SalaryChanged event is emitted with just the block.timestamp and _newSalary, leaving no record of which address the salary was changed for.

Include the _address as a part of the SalaryChanged event data.

• The address is now included in the SalaryChanged event data.

The allocations[msg.sender].totalPayout = allocations[msg.sender].totalPayout.add(amount) computation inefficiently references the allocations state data twice.

Use += in order to save gas.

• += is now used.

The comment states “Public Functions” yet initialize directly below is internal.

Move comment to where the section below is solely public functions.

• The comment has been updated.

The mapping addressToId is really the contract id to the address of the contract.

Change name to idToAddress.

• The name of the mapping has been updated.

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

Update the comment to reflect what the function does.

• The comment has been updated.

isResultReported is not set to true after the call to reportResult. Therefore, the result can be reported multiple times with varying arguments. Furthermore, line 119 isVotingClosed = false can be used to prevent a winner from ever getting picked in pickWinner().

Add isResultReported = true to the end of the function.

• The suggested addition was made.

In the resultVote function, the if statements that determines which finalVoteDecision to return are placed inside of the for loop that counts the votes. Therefore the finalVoteDecision will be arbitrarily based on whichever votes happen to be first in the votes list.

Move the if statements determining finalVoteDecision after the for loop, or preferably refactor the voting entirely per SQDF-3.

• The if statement has been moved outside of the for loop

The for loop is reliant on the number of votes. If the number of votes is very high, the calculation may exceed the block gas limit and fail.

Another way to approach this problem is to have an int variable that increases by 1 when the player votes one way and decreases by 1 when the player votes the other way in Vote. The sign of the end value once voting is finished will tell you which choice had more votes.Therefore, a for loop can be entirely avoided.

• The suggested approach was implemented.

reportResult does not need to contain logic for transferring funds to the prize pool because transferTotalEntryFeestoPrizePool exists.

Remove transfer logic from reportResult.

• Transfer logic was removed from reportResult.

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

Utilize a strong source of randomness whether it be the on-chain randomness pattern or an oracle.

• Randomness will be secured with the implementation of Chainlink VRF.

registerPlayer does not check if registration exceeds the maxNumberOfPlayers. As a result, contracts that loop over the winners such as SquidBetPrizePool may face a DoS attack.

Add the check in registerPlayer.

• The suggested requires has been added.

The betting fee is unable to be updated because updateBettingFees updates the maxNumberofPlayers rather than bettingFee.

Change the function body to bettingFee = _newBettingFee.

• The function now updates bettingFee.

winnersClaimPrizePool uses a push pattern such that it loops over all winners and sends them their funds. If there are too many equal winners then no one would be able to claim due to the for loop exceeding the block gas limit. Furthermore, if a winner is a contract that is unable to receive ether upon .transfer(), no one would be able to receive their prize money.

Utilize a pull-over-push pattern such that when a user calls winnersClaimPrizePool, they receive solely their funds and no other funds are dispatched.

• A pull over push pattern was introduced.

Anyone can call the function winnersClaimPrizePool and end the prize pool as long as some winners have been added.

Make sure to always add all winners at once.

• The risk has been acknowledged.