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

Receiver and msg.sender Mismatch in deposit() Causes Accounting Errors

jufel

Description:

The deposit() function has a critical design flaw where it accepts a receiver parameter but inconsistently uses both receiver and msg.sender throughout the function:

function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0));
// ...
stakedAsset[receiver] = stakeAsset; // Uses receiver
// ...
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee); // Uses msg.sender
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset); // Uses msg.sender
_mint(msg.sender, participantShares); // Mints shares to msg.sender, NOT receiver!
// ...
}

The problems:

  1. stakedAsset[receiver] is set, but shares are minted to msg.sender

  2. If receiver != msg.sender, the receiver gets credit for the deposit but msg.sender gets the shares

  3. The receiver cannot use joinEvent() because they have no shares, even though stakedAsset[receiver] > 0

  4. The msg.sender has shares but stakedAsset[msg.sender] == 0, so they also cannot join

Impact:

  • If a user calls deposit(1000, alice) intending alice to be the receiver:

    • stakedAsset[alice] = 990 (after fees)

    • Shares are minted to msg.sender, not alice

    • Alice cannot call joinEvent() because balanceOf(alice) == 0

    • msg.sender cannot call joinEvent() because stakedAsset[msg.sender] == 0

  • Complete broken functionality when receiver != msg.sender

  • Users lose the ability to participate in betting

  • Shares and staked amounts become desynchronized

Proof of Concept:

function testReceiverMsgSenderMismatch() public {
address alice = makeAddr("alice");
​
// Attacker deposits with alice as receiver
vm.startPrank(attacker);
asset.approve(address(vault), 10000 * 10**18);
uint256 shares = vault.deposit(10000 * 10**18, alice);
vm.stopPrank();
​
// Alice has staked amount credited
assertEq(vault.stakedAsset(alice), 9900 * 10**18, "Alice has staked amount");
​
// But shares went to attacker
assertEq(vault.balanceOf(alice), 0, "Alice has no shares");
assertEq(vault.balanceOf(attacker), shares, "Attacker has shares");
​
// Alice cannot join because she has no shares
vm.prank(alice);
// joinEvent checks balanceOf(msg.sender) indirectly via userSharesToCountry calculation
// But stakedAsset check passes
vault.joinEvent(0);
​
// Now alice is registered but with 0 shares
assertEq(vault.userSharesToCountry(alice, 0), 0, "Alice joined with 0 shares");
​
// Attacker has shares but cannot join
vm.prank(attacker);
vm.expectRevert(BriVault.noDeposit.selector);
vault.joinEvent(1); // Fails because stakedAsset[attacker] == 0
}

Mitigation:

The function should only use msg.sender for consistency, or properly handle the receiver pattern:

Option 1 (Recommended): Remove receiver parameter and use msg.sender:

function deposit(uint256 assets) public returns (uint256) {
address receiver = msg.sender;
​
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 fee = _getParticipationFee(assets);
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
​
uint256 stakeAsset = assets - fee;
stakedAsset[msg.sender] = stakeAsset;
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(msg.sender, stakeAsset);
return participantShares;
}

Option 2: Consistently use receiver throughout:

function deposit(uint256 assets, address receiver) public override returns (uint256) {
require(receiver != address(0));
​
if (block.timestamp >= eventStartDate) {
revert eventStarted();
}
​
uint256 fee = _getParticipationFee(assets);
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
​
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset; // Set for receiver
uint256 participantShares = _convertToShares(stakeAsset);
​
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
_mint(receiver, participantShares); // Mint to receiver
​
emit deposited(receiver, stakeAsset);
return 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!