BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Players can deposit on behalf of others but still keep shares for their own.

noahzzz

Root + Impact

Description

  • Players can deposit on behalf of others but keep all shares to themselves since shares are minted to msg.sender in deposit process, instead of receiver.

  • Hence when winner is set and victims want to withdraw their prize, they will get zero payout since they don't have any shares under their address.

  • This BriVault::deposit does not conform to ERC-4626 semantics (mint to receiver, notmsg.sender).

function deposit(uint256 assets, address receiver) public override returns (uint256) {
​
...
​
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
// @> mint to msg.sender instead of receiver
_mint(msg.sender, participantShares);
​
emit deposited (receiver, stakeAsset);
return participantShares;
}
​

Risk

Likelihood:

  • No permissions needed. Attacker just calls deposit(x, victim) and keeps the shares. The only cost is gas and participation fee.

Impact:

  • Potential griefing risk: An attacker can “sponsor” deposits for many victims, and make those victims think they’re in—yet they hold no shares, so even if their country wins they withdraw nothing.

  • Attackers don't directly steal from victim's wallet or the vault, but misled victims may lose expected payout.

Proof of Concept

The test function below shows the attack flow for delegated deposit. Player2 is misled by malicious player1 to think they already has the shares. Player2 can withdraw nothing after winner is set, even though they are a legit winner.

function test_exploitCanDepositOnBehalfOfOthers() public {
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
​
mockToken.mint(player1, 50 ether);
mockToken.mint(player2, 50 ether);
​
vm.prank(owner);
briVault.setCountry(countries);
​
vm.startPrank(player1);
mockToken.approve(address(briVault), type(uint256).max);
// on behalf of player2 but keep all shares to self
briVault.deposit(5 ether, player2);
// deposit little for self to join
briVault.deposit(0.001 ether, player1);
briVault.joinEvent(0);
vm.stopPrank();
​
vm.startPrank(player2);
// player2 can join without deposit
briVault.joinEvent(0);
vm.stopPrank();
​
vm.warp(eventEndDate + 1);
vm.prank(owner);
briVault.setWinner(0);
​
vm.prank(player2);
briVault.withdraw();
// player2 gets 0 prize since all shares are owned by player1
assertEq(mockToken.balanceOf(player2), 50 ether);
}

How to run this test: Paste test function test_exploitCanDepositOnBehalfOfOthers in file test/briVault.t.sol.

Recommended Mitigation

Change the minting logic to make it conform with ERC-4626 semantics (mint to receiver, notmsg.sender).

- _mint(msg.sender, participantShares)
+ _mint(receiver, participantShares)
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Shares Minted to msg.sender Instead of Specified Receiver

Support

FAQs

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

Give us feedback!