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

Missing Test: Only owner can set winner

yogiemoji

Root + Impact

Description

  • Describe the normal behavior in one or more sentences: Only the owner can set the winner once the tournament event has completed.

  • Explain the specific issue or problem in one or more sentences: There is no test to verify this is the case.

// This is the code section in the BriVault.sol file, lines 123-148
function setWinner(
uint256 countryIndex
) public onlyOwner returns (string memory) {
if (block.timestamp <= eventEndDate) {
revert eventNotEnded();
}
​
require(countryIndex < teams.length, "Invalid country index");
​
if (_setWinner) {
revert WinnerAlreadySet();
}
​
winnerCountryId = countryIndex;
winner = teams[countryIndex];
​
_setWinner = true;
​
_getWinnerShares();
​
_setFinallizedVaultBalance();
​
emit WinnerSet(winner);
​
return winner;
}
// However in BriVault.t.sol, no test ensures this functions works correctly.

Risk

Likelihood:

  • Reason 1 // Describe WHEN this will occur (avoid using "if" statements): When the above function is run, no test checks that the owner is the person selecting the winning team.

  • Reason 2 I've set the likelihood to low as after running tests (using the mitigation below) there was no failure in the test.

Impact:

  • Impact 1: When this function fails, any user could set themselves as the winner.

  • Impact 2: All funds lost to non-winners.

Proof of Concept

## Proof of Concept for NotOwnerCannotSetWinner
### Overview: There is no test in the BriTest.t.sol file to ensure
that the owner can only set the winner.
### Actors:
- **Attacker**: Withdraw funds from briVault.
- **Victim**: BriVault and owner and winners, the latter doesn't get their winnings.
- **Protocol**: Protocol should prevent nonowners setting the winner
### Missing Test: Only owner can set winner:
See below mitigation to resolve.

// I don't believe the risk would occur as the test below passed.

//I also ran a fuzz test to be sure which also passed,

// due to the onlyOwner function being a general access control mechanism.

Recommended Mitigation

Adding the test below ensures that we're checking only the owner can set the winner.

// Add to BriVault.t.sol
+function testNotOwnerCannotSetWinner() public {
+ vm.prank(owner);
+ briVault.setCountry(countries);
+ vm.warp(eventEndDate + 1); // Move past event end
+ vm.prank(user1);
+ vm.expectRevert();
+ briVault.setWinner(2); // Try to set Mexico as winner
+ }
Updates

Appeal created

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

The winner is set by the owner

This is owner action and the owner is assumed to be trusted and to provide correct input arguments.

Support

FAQs

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

Give us feedback!