Root + Impact
The setCountry() function has no validation to prevent duplicate team names in the teams array, which breaks the winner determination logic and causes incorrect payouts.
Description
Normal behavior expects each team in a tournament to have a unique identifier. Users select teams by index, and the winner is determined by matching team names.
The current implementation allows the owner to set duplicate team names in different indices. This causes multiple indices to match the winner string, breaking the assumption that each team is unique.
function setCountry(string[48] memory countries) public onlyOwner {
for (uint256 i = 0; i < countries.length; ++i) {
teams[i] = countries[i];
}
emit CountriesSet(countries);
}
function joinEvent(uint256 countryId) public {
userToCountry[msg.sender] = teams[countryId];
userSharesToCountry[msg.sender][countryId] = participantShares;
}
function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
winner = teams[countryIndex];
}
function withdraw() external winnerSet {
if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
}
Risk
Likelihood:
-
Owner can accidentally set duplicates due to typos or copy-paste errors
-
No on-chain validation prevents this during setup
-
Easy to make mistake when setting 48 team names manually
Impact:
-
If teams[0] = "TeamA" and teams[5] = "TeamA" (duplicate)
-
Users joining index 0 set userSharesToCountry[user][0] = shares
-
Users joining index 5 set userSharesToCountry[user][5] = shares
-
Owner sets winner to index 0: winner = "TeamA"
-
_getWinnerShares() counts shares for BOTH index 0 and 5
-
totalWinnerShares is artificially inflated
-
Both groups can withdraw but payouts are diluted incorrectly
-
Creates confusion about which "TeamA" actually won
-
Breaks tournament fairness and integrity
Proof of Concept
string[48] memory teams;
teams[0] = "TeamA";
teams[1] = "TeamB";
teams[2] = "TeamA";
vault.setCountry(teams);
alice.deposit(1000e18, alice);
alice.joinEvent(0);
bob.deposit(1000e18, bob);
bob.joinEvent(2);
vault.setWinner(0);
Recommended Mitigation
function setCountry(string[48] memory countries) public onlyOwner {
+ // Validate no duplicates
+ for (uint256 i = 0; i < countries.length; ++i) {
+ // Skip empty strings
+ if (bytes(countries[i]).length == 0) continue;
+
+ for (uint256 j = i + 1; j < countries.length; ++j) {
+ require(
+ keccak256(abi.encodePacked(countries[i])) !=
+ keccak256(abi.encodePacked(countries[j])),
+ "Duplicate team names not allowed"
+ );
+ }
+ }
+
for (uint256 i = 0; i < countries.length; ++i) {
teams[i] = countries[i];
}
emit CountriesSet(countries);
}
Better approach: Use calldata and prevent calling after deposits
-function setCountry(string[48] memory countries) public onlyOwner {
+function setCountry(string[48] calldata countries) external onlyOwner {
+ // Prevent changing teams after deposits started
+ require(totalSupply() == 0, "Cannot change teams after deposits");
+
+ // Validate no duplicates
+ for (uint256 i = 0; i < countries.length; ++i) {
+ if (bytes(countries[i]).length == 0) continue;
+
+ for (uint256 j = i + 1; j < countries.length; ++j) {
+ require(
+ keccak256(abi.encodePacked(countries[i])) !=
+ keccak256(abi.encodePacked(countries[j])),
+ "Duplicate team names not allowed"
+ );
+ }
+ }
+
for (uint256 i = 0; i < countries.length; ++i) {
teams[i] = countries[i];
}
emit CountriesSet(countries);
}
