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

Shares are minted to msg.sender, but stake is recorded for receiver

themilenkov

Inconsistent Accounting and Overwritten Deposits in deposit() Break Stake Tracking and Refund Logic

Description

  • The deposit logic contains two interrelated issues that break internal accounting consistency between deposited assets and minted vault shares:

    1. Shares minted to msg.sender, stake recorded for receiver

  • Shares (BTT tokens) are minted to msg.sender, but the deposit record (stakedAsset) is written under receiver.

  • Functions such as joinEvent() and cancelParticipation() check stakedAsset[msg.sender] to validate deposits and calculate refunds, meaning:

    • If receiver != msg.sender, the recorded user cannot cancel or join because they hold no shares.

    • The depositor (msg.sender) cannot cancel either, because stakedAsset[msg.sender] is unset.

    • If receiver calls cancelParticipation(), they can withdraw funds without burning any shares, leaving unbacked shares in circulation.


    2. Multiple deposits overwrite previous stakedAsset value

  • A second deposit replaces the previous amount instead of accumulating. When the user later cancels participation

  • As a result, the contract refunds only the most recent deposit amount while burning all user shares — effectively burning funds from the participant’s balance and leaving tokens stranded inside the vault.

/**
@dev allows users to deposit for the event.
*/
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; // overwrites previous deposit
​
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;
}

Likelihood:

  • Triggered under normal user interactions (multiple deposits, mismatched receiver, or front-end misconfiguration).

Impact:

  • Causes accounting desynchronization between recorded stakes and shares, leading to incorrect refunds, lost deposits, and unbacked shares in the system.

Proof of Concept

Scenario:

  1. Alice deposits 100 tokens (receiver = Alice) → stakedAsset[Alice] = 100.

  2. Alice deposits another 200 tokens (receiver = Alice) → stakedAsset[Alice] = 200 (overwrites old).

  3. Alice now holds shares representing 300 tokens total, but stakedAsset[Alice] = 200.

  4. When she calls cancelParticipation(), it refunds only 200 tokens while burning all 300 tokens worth of shares.

  5. 100 tokens remain stuck in the vault, unclaimable.

If instead Bob deposits with receiver = Alice, Alice will later be able to withdraw her full refund (using stakedAsset[Alice]) without burning any shares, leaving unbacked BTT shares in supply.

Paste the following test into briVault.t.sol:

function test_depositOverwritesStake() public {
vm.startPrank(owner);
briVault.setCountry(countries);
vm.stopPrank();
​
vm.startPrank(user1);
mockToken.approve(address(briVault), 10 ether);
​
// First deposit
briVault.deposit(5 ether, user1);
// Second deposit overwrites mapping
briVault.deposit(5 ether, user1);
​
// stakedAsset only tracks last deposit
assertEq(briVault.stakedAsset(user1), 4.925 ether); // 5 - fee
​
// cancelParticipation() refunds only last deposit
uint256 before = mockToken.balanceOf(user1);
briVault.cancelParticipation();
uint256 refunded = mockToken.balanceOf(user1) - before;
​
assertLt(refunded, 2 * 4.925 ether, "Refund too small due to overwrite");
vm.stopPrank();
}

Recommended Mitigation

Ensure consistent ownership — Mint shares and record stake for the same address:

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;
- uint256 participantShares = _convertToShares(stakeAsset);
-
-
- IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
-
- IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
-
- _mint(msg.sender, participantShares);
+ // Accumulate deposits instead of overwriting previous value
+ stakedAsset[receiver] += stakeAsset;
+
+ uint256 participantShares = _convertToShares(stakeAsset);
+
+ // Transfer tokens and mint shares consistently with ERC4626 semantics
+ IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
+ IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
+
+ // Mint shares to receiver, not msg.sender, to align with recorded stake
+ _mint(receiver, participantShares);
​
emit deposited (receiver, stakeAsset);
​
return participantShares;
}
Updates

Appeal created

bube Lead Judge 20 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!