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

Team Name Mutability Breaks Winner Withdrawal

0xnaresh

Team Name Mutability Breaks Winner Withdrawal

Description

  • Users deposit assets and call joinEvent(countryId) to select a team from the teams[] array. The contract records their choice by storing the team name as a string in userToCountry[msg.sender]. When the owner calls setWinner(countryIndex), the winner variable is set to the current value of teams[countryIndex]. Withdrawal eligibility is verified by comparing these two strings using keccak256.

  • The issue arises because setCountry() can be called at any time by the owner, modifying teams[] after users have already joined. This causes userToCountry to hold an outdated team name, while winner reflects the updated name. The string comparison in withdraw() fails even when the user selected the correct team index.

// @> Root cause: storing mutable team name as string
userToCountry[msg.sender] = teams[countryId]; // <- snapshot at join time
​
// @> Root cause: winner uses current (mutable) team name
winner = teams[countryIndex];
​
// @> Root cause: withdrawal compares strings, not IDs
if (keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
keccak256(abi.encodePacked(winner))) {
revert didNotWin(); // <- fails if name changed
}

Risk

Likelihood:

  • Owner updates team names after users join // occurs during typo fixes, branding changes, or localization

  • Any change to teams[] after joinEvent calls // happens in normal admin workflows

Impact:

  • Legitimate winners are permanently denied withdrawal // funds remain locked in vault

  • All prior participants lose access to rewards // even with correct prediction

Proof of Concept:

A user joins when team[0] is "Brazil", then the owner updates it to "Brasil". After setWinner(0), the user’s stored country ("Brazil") no longer matches winner ("Brasil"), so withdraw() reverts with didNotWin() despite selecting the correct index. 1000 tokens remain permanently locked in the vault.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import "forge-std/Test.sol";
import "../src/briVault.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract MockAsset is ERC20 {
constructor() ERC20("Mock Asset", "MOCK") {
_mint(msg.sender, 1000000 * 10**18);
}
}
contract BriVaultMutability is Test {
BriVault vault;
MockAsset asset;
address alice = address(0xA);
address feeAddr = address(0xF);
uint256 startDate = block.timestamp + 1 days;
uint256 endDate = block.timestamp + 2 days;
function setUp() public {
asset = new MockAsset();
asset.transfer(alice, 1000);
vault = new BriVault(IERC20(asset), 0, startDate, feeAddr, 0, endDate);
}
function testTeamChangeBreaksWithdraw() public {
// Set initial countries
string[48] memory countries;
countries[0] = "Brazil";
vault.setCountry(countries);
// Alice deposits and joins
vm.startPrank(alice);
asset.approve(address(vault), 1000);
vault.deposit(1000, alice);
vault.joinEvent(0); // Joins "Brazil"
assertEq(vault.userToCountry(alice), "Brazil");
vm.stopPrank();
// Owner changes team name (e.g: typo fix)
countries[0] = "Brasil";
vault.setCountry(countries);
// Advance time, set winner to index 0 (now "Brasil")
vm.warp(endDate + 1);
vault.setWinner(0);
assertEq(vault.getWinner(), "Brasil");
// Alice tries to withdraw: fails because "Brazil" != "Brasil"
vm.prank(alice);
vm.expectRevert(BriVault.didNotWin.selector);
vault.withdraw();
// Funds stuck
assertEq(asset.balanceOf(address(vault)), 1000);
}
}

Recommended Mitigation:

Stop using team names. Use team numbers (indexes)

## Recommended Mitigation
```diff
- mapping (address => string) public userToCountry;
+ // @> Use country index instead of mutable string to avoid name-change breakage
+ mapping (address => uint256) public userCountryId;
​
- userToCountry[msg.sender] = teams[countryId];
+ // @> Store the selected country index (immutable)
+ userCountryId[msg.sender] = countryId;
​
- if (
- keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
- keccak256(abi.encodePacked(winner))
- ) {
- revert didNotWin();
- }
+ // @> Compare country IDs – immune to team name updates
+ if (userCountryId[msg.sender] != winnerCountryId) {
+ revert didNotWin();
+ }
Updates

Appeal created

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

setCountry() Can Be Called After Users Join

This is owner action.

Support

FAQs

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

Give us feedback!