BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: low
Invalid

No guard on duplicate country names

neomartis

No guard on duplicate country names

Description

  • Normal behavior: Each country should have a unique identifier and verification should be based on immutable indices. Names are metadata and should not allow multiple different indices to be treated as the same winner.

  • Issue: The contract allows duplicate country names via setCountry. The guarded withdraw() checks string equality (userToCountry vs winner), while totalWinnerShares is computed using the winner’s index (winnerCountryId). If two different indices share the same name, users who picked the non-winner index but with the same name can pass the string check and withdraw, while the denominator (totalWinnerShares) only includes shares for the true winner index. This causes overpayment and drains the vault.

// @> allows duplicates in country names; no validation performed
function setCountry(string[48] memory countries) public onlyOwner {
...
}
​
// @> verification uses string equality instead of immutable index
function withdraw() external winnerSet {
...
if (
keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
...
}
​
// @> denominator is computed from the winner index only
function _getWinnerShares () internal returns (uint256) {
for (uint256 i = 0; i < usersAddress.length; ++i){
address user = usersAddress[i];
totalWinnerShares += userSharesToCountry[user][winnerCountryId];
}
return totalWinnerShares;
}

Risk

Likelihood: Low

  • Requires owner to set duplicates.

  • Still unguarded today and easy to misconfigure.

Impact: High

  • Users from the non-winner index with the same name can withdraw, causing overpayment and exhausting the vault.

  • Legitimate winners may fail to claim due to insufficient remaining assets; funds can be locked for later claimants.

Proof of Concept

Description:

  • Owner sets countries with duplicate names at indices 7 and 8.

  • Users join both indices 7 and 8; owner sets winner to index 7.

  • Both groups pass the string-equality check and withdraw, but totalWinnerShares only counted index 7, causing the vault to underflow for later winners.

function testNoGuardOnDuplicatedCountryNames() public {
countries[7] = "Duplicate";
countries[8] = "Duplicate";
​
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user1);
briVault.joinEvent(7);
vm.stopPrank();
​
vm.startPrank(user2);
mockToken.approve(address(briVault), 1 ether);
briVault.deposit(1 ether, user2);
briVault.joinEvent(8);
vm.stopPrank();
​
vm.startPrank(user3);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user3);
briVault.joinEvent(7);
vm.stopPrank();
​
vm.startPrank(user4);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user4);
briVault.joinEvent(8);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
​
vm.startPrank(owner);
briVault.setWinner(7);
vm.stopPrank();
​
// All users are considered winners because of string equality check, even though
// only index 7 is the winner, this is due to missing validation on setting duplicated
// country names
​
// Now it becomes a race who can claim the funds first
vm.startPrank(user1);
briVault.withdraw();
vm.stopPrank();
​
vm.startPrank(user2);
briVault.withdraw();
vm.stopPrank();
​
vm.startPrank(user3);
// Reverts on insufficient balance
vm.expectRevert(
abi.encodeWithSignature(
"ERC20InsufficientBalance(address,uint256,uint256)",
address(briVault),
7880000000000000000,
9850000000000000000
)
);
briVault.withdraw();
vm.stopPrank();
​
// Only user 1 and user 2 have claimed their balance
console.log("balance user 1", mockToken.balanceOf(user1));
// 20 074545454 545454545 (profit)
console.log("balance user 2", mockToken.balanceOf(user2));
// 20 074545454 545454545 (profit)
console.log("balance user 3", mockToken.balanceOf(user3));
// 15 000000000 000000000 (loss, and cannot claim win)
console.log("vault balance", mockToken.balanceOf(address(briVault)));
// 7 880000000 000000000 (Locked up in the vault, as it is too little to payout user 3 or 4 for their win)
}

Recommended Mitigation

  • Enforce unique country names on setCountry to prevent configuration mistakes.

  • Or better, prefer index-based verification for payout (do not rely on string equality). Use the snapshot of winning shares per user and winner index.

function setCountry(string[48] memory countries) public onlyOwner {
+ // validate uniqueness
+ for (uint256 i = 0; i < countries.length; ++i) {
+ require(bytes(countries[i]).length != 0, "empty name");
+ for (uint256 j = i + 1; j < countries.length; ++j) {
+ require(
+ keccak256(abi.encodePacked(countries[i])) != keccak256(abi.encodePacked(countries[j])),
+ "duplicate country name"
+ );
+ }
+ }
for (uint256 i = 0; i < countries.length; ++i) {
teams[i] = countries[i];
}
emit CountriesSet(countries);
}

Index-based payout (avoid strings):

- if (keccak256(abi.encodePacked(userToCountry[msg.sender])) != keccak256(abi.encodePacked(winner))) {
- revert didNotWin();
- }
+ require(userSharesToCountry[msg.sender][winnerCountryId] != 0, "didNotWin()");
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!