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

deposit() mints shares to depositor while staking credited to receiver, breaking accounting consistency

shashankwcw

When A calls deposit(X, B), the vault should take tokens from A (pay fees), and credit both the underlying stake and the minted shares to the same beneficiary (either A or B).So, when a user cancels his participation via cancelParticipation() his funds are stuck in the contract. If the call of cancelParticipation() isn't made by the receiver address passed in the deposit() function

Description

  • The function assigns the deposited amount to receiver but mints shares to msg.sender. In the lines stakedAsset[receiver] = stakeAsset; _mint(msg.sender, participantShares);

This creates an accounting mismatch — the recorded staker and the share owner are not the same entity.

  • Such inconsistent state breaks vault accounting and may lead to:

    • Locked or unclaimable funds.

    • Users holding shares not backed by deposits.

    • Potential exploitation where a user gains shares without economic exposure.

// Deposit Function
function deposit(uint256 assets, address receiver) public override returns (uint256) {
// Rest of the function ////////////
@> 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);
​
emit deposited (receiver, stakeAsset);
​
return participantShares;
}

Risk

Likelihood:

  • This issue occurs whenever deposit() is called with receiver != msg.sender, which is a valid and unrestricted input in the function. No specific preconditions or external circumstances are required for the inconsistency to appear.

  • The inconsistent logic is embedded in the normal deposit flow and affects all deposits-for-others, meaning it is deterministic and reproducible under standard operation, as confirmed by the test_mint_stake_confusion results.

Impact:

  • The vault’s internal accounting becomes inconsistent — the address holding shares (entitled to rewards/withdrawals) differs from the address recorded as staking the assets (entitled to refunds). This breaks core ERC4626 and vault accounting assumptions.

  • Depending on subsequent user actions, this can lead to loss of funds or unclaimable assets. A depositor can mint shares without risk exposure, or the receiver’s stake can remain permanently locked, resulting in vault insolvency or incorrect fund distribution.

Proof of Concept

  • This test shows that if the depositor and receiver address are different funds are stuck

function test_mint_stake_confusion() public {
vm.startPrank(user1);
mockToken.approve(address(briVault), 5 ether);
briVault.deposit(2 ether, user2);
vm.stopPrank();
uint stakedAssetb4 = briVault.stakedAsset(user2);
uint balance_token_before = mockToken.balanceOf(user1);
​
​
vm.startPrank(user1);
briVault.cancelParticipation();
uint stakedAssetafter = briVault.stakedAsset(user2);
uint balance_token_after = mockToken.balanceOf(user1);
vm.stopPrank();
console.log("Staked asset before cancel:", stakedAssetb4);
console.log("Staked asset after cancel:", stakedAssetafter);
console.log("Balance before cancel:", balance_token_before);
console.log("Balance after cancel:", balance_token_after);
​
}
// Output
[PASS] test_mint_stake_confusion() (gas: 171665)
Logs:
Staked asset before cancel: 1970000000000000000
Staked asset after cancel: 1970000000000000000
Balance before cancel: 19998000000000000000000
Balance after cancel: 19998000000000000000000
​
Traces:
├─ [936] BriVault::stakedAsset(user2: [0x537C8f3d3E18dF5517a58B3fB9D9143697996802]) [staticcall]
│ └─ ← [Return] 1970000000000000000 [1.97e18]
├─ [850] MockERC20::balanceOf(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 19998000000000000000000 [1.999e22]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("Staked asset before cancel:", 1970000000000000000 [1.97e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Staked asset after cancel:", 1970000000000000000 [1.97e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Balance before cancel:", 19998000000000000000000 [1.999e22]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("Balance after cancel:", 19998000000000000000000 [1.999e22]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.33ms (386.10µs CPU time)
​
Ran 1 test suite in 22.62ms (3.33ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • Just mint the shares for the receiver address

- _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!