Arena Contract Review

[drmathias]

Hi, I took some time to review the Arena contract and left my findings below:

---

Battle Id is not validated, allowing battle to be overwritten. Consider storing an incremental counter for battle id in the contract and returning battle id from method.

HashBattle/HashBattle/Arena.cs

Line 37 in 8fbe90d

SetBattle(battleId, battle);

---

User index is not validated, making it possible for anyone to overwrite a battle user. Consider setting user index from an incremental counter.

HashBattle/HashBattle/Arena.cs

Line 62 in 8fbe90d

battle.Users.SetValue(user.Address, userindex);

---

If scores are tied, the user with a lower index will win and the prize will not be split. Is this intended?

HashBattle/HashBattle/Arena.cs

Line 139 in 8fbe90d

if (user.Score > winningScore)

---

Condition will always be true, as users array is always initialised with a length of 4. Remove to lower gas cost.

HashBattle/HashBattle/Arena.cs

Line 111 in 8fbe90d

if (battle.Users.Length <= 4)

---

Condition will always be true, as was previously asserted. Remove to lower gas cost.

HashBattle/HashBattle/Arena.cs

Line 121 in 8fbe90d

if (battle.Winner == Address.Zero)

---

Prize value considers battle has 4 users. If the battle ends without 4 users, the transfer will fail. Is it intended that every battle must have 4 users?

HashBattle/HashBattle/Arena.cs

Line 154 in 8fbe90d

ulong prize = battle.Fee * (battle.MaxUsers - 1);

---

Methods returning bool will always return true. Consider making return type void instead.

HashBattle/HashBattle/Arena.cs

Line 66 in 8fbe90d

return true;