Submission Details
Impact:
Likelihood:
Using the winning team's name as a string instead of the index in the verification process during the withdrawal, Lead to money being withdrawn from losing Participants.
Using the winning team's name as a string instead of the index in the verification process during the withdrawal, Lead to money being withdrawn from losing Participants.
Description
When the owner enters the 48 teams calling
brivault::setCountry, there is nothing preventing the same name from insertion more than once, And because the verification that the participant chose the winning team is done by string name and not index during the withdrawal process, if someone bets on a team in index 5 and the winning team in index 10 has the same name, participants who bet on the losing team in index 5 will still be able to withdraw funds just like the participants who actually won.
function withdraw() external winnerSet {
if (block.timestamp < eventEndDate) {
revert eventNotEnded();
}
@> if (
@> keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
@> keccak256(abi.encodePacked(winner))
) {
revert didNotWin();
}
uint256 shares = balanceOf(msg.sender);
uint256 vaultAsset = finalizedVaultAsset;
uint256 assetToWithdraw = Math.mulDiv(shares, vaultAsset, totalWinnerShares);
_burn(msg.sender, shares);
IERC20(asset()).safeTransfer(msg.sender, assetToWithdraw);
emit Withdraw(msg.sender, assetToWithdraw);
}
Risk
Likelihood:
It might occur under specific conditions. For example, The team name is repeated in the array, and the participant bets on the losing team's name, which is the same as the winning team's name.
Impact:
-
Funds are indirectly at risk.
Proof of Concept
In this next test, we will make the team in Index 5 and Index 10 have the same name, and have user number 1 bet on the losing team in Index 5, while users number 2 through 5 bet on the winning team in Index 10. Then, we will withdraw the funds from the losing user number 1.
Put this test in `briVault.t.sol`
<details>
<summary>POC</summary>
```javascript
function test_duplicateTeamName_vulnerability() public {
// 1) Owner sets countries with duplicate name: make index 10 equal to index 5
vm.startPrank(owner);
string[48] memory dupCountries = countries;
dupCountries[10] = dupCountries[5]; // duplicate the name at index 10
briVault.setCountry(dupCountries);
vm.stopPrank();
// 2) Everyone deposits 10 ether and joins:
// user1 -> index 5 (will be 'loser' but name duplicated)
vm.startPrank(user1);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user1);
briVault.joinEvent(5);
vm.stopPrank();
// user2 -> index 10 (winner group)
vm.startPrank(user2);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user2);
briVault.joinEvent(10);
vm.stopPrank();
// user3 -> index 10
vm.startPrank(user3);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user3);
briVault.joinEvent(10);
vm.stopPrank();
// user4 -> index 10
vm.startPrank(user4);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user4);
briVault.joinEvent(10);
vm.stopPrank();
// user5 -> index 10
vm.startPrank(user5);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user5);
briVault.joinEvent(10);
vm.stopPrank();
// 3) Warp to after event end and owner sets winner as index 10
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
string memory winning = briVault.setWinner(10);
console.log("Winning team (index 10):", winning);
console.log("Finalized vault asset:", briVault.finalizedVaultAsset());
vm.stopPrank();
// 4) user1 (who originally joined index 5) tries to withdraw
vm.startPrank(user1);
uint256 beforeBal = mockToken.balanceOf(user1);
// If vulnerability exists, this call will succeed and user1 will receive some funds
// (expected behaviour if vault matches by name incorrectly)
briVault.withdraw();
uint256 afterBal = mockToken.balanceOf(user1);
uint256 withdrawn = afterBal - beforeBal;
console.log("user1 withdrawn amount (wei):", withdrawn);
vm.stopPrank();
// 5) assert withdrawn > 0 to prove vulnerability (expectation: vulnerable behavior)
assertTrue(withdrawn > 0, "Expected user1 to withdraw > 0 due to duplicate name vulnerability");
}
```
</details>
Recommended Mitigation
1. Rewrite `brivault::setCountry` like this.
```diff
function setCountry(string[48] memory countries) public onlyOwner {
- for (uint256 i = 0; i < countries.length; ++i) {
- teams[i] = countries[i];
- }
- emit CountriesSet(countries);
+ for (uint256 i = 0; i < countries.length; ++i) {
+ for (uint256 j = i + 1; j < countries.length; ++j) {
+ if (keccak256(bytes(countries[i])) == keccak256(bytes(countries[j]))) {
+ revert("duplicateCountry()");
+ }
+ }
+ teams[i] = countries[i];
+ }
+ emit CountriesSet(countries);
}
```
Rewards breakdown
nSLOC
193This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.