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

Custom _convertToShares would not mint 1:1 as intended if a donation is made to the vault beforehand

ynyesto

Description

The vault has a custom _convertToShares function that aims at minting 1 share per deposited token to the first depositor, as per the inline comment.

If someone sent tokens to the vault (calling the ERC20's transfer function) the first depositor would mint more than one share per token.

function _convertToShares(uint256 assets) internal view returns (uint256 shares) {
// @audit-low: don't reinvent the wheel, use the ERC4626 contract's _convertToShares() function for this
uint256 balanceOfVault = IERC20(asset()).balanceOf(address(this));
uint256 totalShares = totalSupply(); // total minted BTT shares so far
​
if (totalShares == 0 || balanceOfVault == 0) {
// First depositor: 1:1 ratio @> this won't hold if first deposit is front run by a donation
return assets;
}
​
shares = Math.mulDiv(assets, totalShares, balanceOfVault);
}

Risk

Likelihood:

  • It seems rather unlikely that someone would just donate their tokens to the contract, since there is no economic incentive.

Impact:

  • First depositor would mint more shares than tokens deposited.

Proof of Concept

The following PoC shows how the first depositor would capture a donation made to the vault before any deposits and subsequent users would just mint the expected amounts.

function testDonationCaptureByFirstDepositor() public {
// 1. Someone donates before any minting
deal(address(mockToken), address(briVault), 1 ether); // simulate donation directly
​
// 2. First user deposits normally
uint256 depositAmount = 5 ether;
vm.startPrank(user1);
mockToken.approve(address(briVault), depositAmount);
uint256 shares = briVault.deposit(depositAmount, user1);
vm.stopPrank();
​
// 3. The vault now holds 6 assets (1 donated + 5 deposited)
// If properly implemented, the first depositor should get shares ~5/6 of total supply
// But a buggy vault with "if totalSupply == 0 -> shares = assets" gives 1:1 mint
// => user gets all vault value (6 assets worth of shares minus fee for 5 deposited)
uint256 userShares = briVault.convertToAssets(shares);
console2.log("User shares:", userShares);
​
// Assertion: the depositor captured donation value
assertGt(userShares, depositAmount); // each share worth > 1 asset, donation captured
​
// Second user deposits normally
vm.startPrank(user2);
mockToken.approve(address(briVault), depositAmount);
uint256 shares2 = briVault.deposit(depositAmount, user2);
vm.stopPrank();
console2.log("User2 shares:", briVault.convertToAssets(shares2));
​
// Third user deposits normally
vm.startPrank(user3);
mockToken.approve(address(briVault), depositAmount);
uint256 shares3 = briVault.deposit(depositAmount, user3);
vm.stopPrank();
console2.log("User3 shares:", briVault.convertToAssets(shares3));
}

Recommended Mitigation

Simply remove the custom implementation, since the standard _convertToShares already handles all edge cases solvently.

- function _convertToShares(uint256 assets) internal view returns (uint256 shares) {
- // @audit-low: don't reinvent the wheel, use the ERC4626 contract's _convertToShares() function for this
- uint256 balanceOfVault = IERC20(asset()).balanceOf(address(this));
- uint256 totalShares = totalSupply(); // total minted BTT shares so far
​
- if (totalShares == 0 || balanceOfVault == 0) {
- // First depositor: 1:1 ratio
- return assets;
- }
​
- shares = Math.mulDiv(assets, totalShares, balanceOfVault);
- }
Updates

Appeal created

bube Lead Judge 19 days ago
Submission Judgement Published
Validated
Assigned finding tags:

Inflation attack

Support

FAQs

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

Give us feedback!