BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

The vault mints shares before updating a total amount of assets causing wrong amount of shares minted and possibility of Inflation Attack

valya

Root + Impact

Description

  • The vault should mint shares based on:

    • the actual vault balance of asset after deposited amount is transferred in, and

    • based on the current exchange rage of shares.

  • The order of logic steps in the deposit function is wrong:
    Step 1: it calculates shares first - the old balance is used for this;
    Step 2: transfers assets from a depositor - updates vault's balance;
    Step 3: mints amount of shares calculated in Step 1.

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);
emit deposited(receiver, stakeAsset);
return participantShares;
}

Risk

Likelihood: HIGH

  • The issue occurs every time the deposit function is called.

Impact: HIGH

  • Shares are calculated wrongly for a depositor - greater amount is minted then it should be.

Proof of Concept

  1. Make a first deposit - amount of shares minted equals amount of stacked asset.

  2. Make a second deposit - amount of shares minted is based on the old vault balance, because this amount of shares is calculated before the actual transfer of assets to the vault.

  3. Make a third deposit - amount of shares minted is based on the old vault balance, the same as during deposit 2.

Add the following test to briVault.t.sol:

function test_wrongOrderOfSharesMinting_possibleInflationAttack() public {
uint256 depositAmount = 5 ether;
uint256 BASE = 10000;
uint256 feeOnDeposit = (depositAmount * briVault.participationFeeBsp()) / BASE;
uint256 stackedAsset = depositAmount - feeOnDeposit;
​
/////////////////
/// DEPOSIT 1 ///
/////////////////
// deposit 1 - mints shares = stacked amount
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
uint256 vaultTotalSupplyBeforeDeposit1 = briVault.totalSupply();
briVault.deposit(depositAmount, user1);
vm.stopPrank();
​
// vault mints shares amount equal to deposited amount minus fee for the first depositor
assertEq(briVault.balanceOf(user1), stackedAsset);
​
/////////////////
/// DEPOSIT 2 ///
/////////////////
// deposit 2 - vault mints shares to the depositor based on old vault balance:
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmount);
uint256 vaultTotalSupplyBeforeDeposit2 = briVault.totalSupply();
uint256 vaultBalanceBeforeDeposit2 = mockToken.balanceOf(address(briVault));
briVault.deposit(depositAmount, user2);
vm.stopPrank();
​
// calculate shares to be minted to user2 based on the actual vault balance
uint256 vaultBalanceAfterDeposit2 = mockToken.balanceOf(address(briVault));
uint256 calculatedSharesToBeMintedAfterDeposit2 =
Math.mulDiv(stackedAsset, vaultTotalSupplyBeforeDeposit2, vaultBalanceAfterDeposit2);
​
// compare actually minted and calculated shares using updated vault balance after deposit 2
uint256 actuallyMintedSharesForUser2 = briVault.balanceOf(user2);
assertGt(actuallyMintedSharesForUser2, calculatedSharesToBeMintedAfterDeposit2);
​
// an old vault balance (the balance before the current deposit) was used by a BriVault logic to mint shares:
uint256 calculatedSharesUsingOldVaultBalance2 =
Math.mulDiv(stackedAsset, vaultTotalSupplyBeforeDeposit2, vaultBalanceBeforeDeposit2);
assertEq(actuallyMintedSharesForUser2, calculatedSharesUsingOldVaultBalance2);
​
/////////////////
/// DEPOSIT 3 ///
/////////////////
// deposit 3 - vault mints shares to the depositor based on old vault balance:
vm.startPrank(user3);
mockToken.approve(address(briVault), depositAmount);
uint256 vaultTotalSupplyBeforeDeposit3 = briVault.totalSupply();
uint256 vaultBalanceBeforeDeposit3 = mockToken.balanceOf(address(briVault));
briVault.deposit(depositAmount, user3);
vm.stopPrank();
​
// calculate shares to be minted to user3 based on the actual vault balance
uint256 vaultBalanceAfterDeposit3 = mockToken.balanceOf(address(briVault));
uint256 calculatedSharesToBeMintedAfterDeposit3 =
Math.mulDiv(stackedAsset, vaultTotalSupplyBeforeDeposit3, vaultBalanceAfterDeposit3);
​
// compare actually minted and calculated shares using updated vault balance after deposit 3
uint256 actuallyMintedSharesForUser3 = briVault.balanceOf(user3);
assertGt(actuallyMintedSharesForUser3, calculatedSharesToBeMintedAfterDeposit3);
​
// an old vault balance (the balance before the current deposit) was used by a BriVault logic to mint shares:
uint256 calculatedSharesUsingOldVaultBalance3 =
Math.mulDiv(stackedAsset, vaultTotalSupplyBeforeDeposit3, vaultBalanceBeforeDeposit3);
assertEq(actuallyMintedSharesForUser3, calculatedSharesUsingOldVaultBalance3);
}

Recommended Mitigation

  1. Mint shares amount based on updated vault balance.

  2. For shares calculation use the actually transferred amount of assets into the vault.

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);
+ uint256 participantShares = _convertToShares(stakeAsset);
_mint(msg.sender, participantShares);
emit deposited(receiver, stakeAsset);
return participantShares;
}
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!