BriVault

First Flight #52

Beginner FriendlySolidity

100 EXP

View results

Previous Next

Submission Details

Severity: high

Valid

Possible inflation attack due to inconsistency in storing shares and assets in `deposit` and `cancelParticipation` functions

valya

Root + Impact

Description

It is possible to specify some address as a receiver in a deposit function. When a participant deposits assets, this amount is stored in a mapping for a receiver, but shares are minted for a msg.sender. The receiver and mgs.sender may be different addresses. When a participant decides to cancelParticipation, the stakedAsset for msg.sender will be set to zero, and all shares will be burned. But if while deposit, the msg.sender was not equal to receiver, then, depending on who calls the cancelParticipation function - 2 ways are possible:
1. If cancelParticipation is called by an address equal to receiver from the deposit:
  1. receiver has stackedAsset -> stackedAssets will be transferred from the vault, and the vault balance will decrease;
  2. receiver does not have shares -> no shares will be burned => shares price drops;
2. If cancelParticipation is called by the address that equals msg.sender from the deposit:
  1. msg.sender doesn't have stackedAssets -> zero assets will be transferred to msg.sender => vault balance will not change;
  2. msg.sender has shares => shares will be burned and shares exchange rate will rise -> possible inflation attack;

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;

}

function cancelParticipation() public {

if (block.timestamp >= eventStartDate) {

revert eventStarted();

}

@> uint256 refundAmount = stakedAsset[msg.sender];

@> stakedAsset[msg.sender] = 0;

uint256 shares = balanceOf(msg.sender);

@> _burn(msg.sender, shares);

IERC20(asset()).safeTransfer(msg.sender, refundAmount);

}

Risk

Likelihood: MEDIUM

The issue occurs when the msg.sender address is not equal to the receiver address in the deposit function.

Impact: HIGH

The issue may cause unpredictable changes of shares exchange rate, excessive shares burn or assets withdrawal.

Proof of Concept

POC proves both cases:

When cancelParticipation is called by the msg.sender:
1. deposit with msg.sender != receiver;
2. Check that the deposited asset amount is stored for the receiver, not for msgSender;
3. Check that shares are minted for msgSender after deposit;
4. Cancel participation by msgSender;
5. Check that the vault balance has not changed after cancelParticipation;
6. Check that all shares are burned from the msg.sender and the vault totalSupply decreased.
When cancelParticipation is called by the receiver:
1. deposit with msg.sender2 != receiver2;
2. Check that the deposited asset amount is stored for receiver2, not for msgSender2;
3. Check that shares are minted for msgSender2 after deposit;
4. Cancel participation by receiver2;
5. Check that the vault balance decreased after cancelParticipation;
6. Check that shares are not burned from msg.sender and the vault totalSupply is not changed.
The separate instances of BriVault are used because there is a bug in the shares calculation in deposit function, so consequent deposits mint the wrong amount of shares.

BriVault briVault2;

function test_msgSenderIsNotEqualToReceiver_whenDeposited() public {

/////////////////////////////////////////////////////////////////////////////////////////

// 'cancelParticipation' is called by address that equals to 'msg.sender' from deposit //

/////////////////////////////////////////////////////////////////////////////////////////

address msgSender = user1;

address receiver = user2;

uint256 depositAmount = 5 ether;

uint256 BASE = 10000;

uint256 feeOnDeposit = (depositAmount * briVault.participationFeeBsp()) / BASE;

uint256 stackedAsset = depositAmount - feeOnDeposit;

vm.startPrank(msgSender);

mockToken.approve(address(briVault), depositAmount);

// 1. deposit with msg.sender != receiver

briVault.deposit(depositAmount, receiver);

// 2. check that deposited asset amount is stored for receiver, not for msgSender

assertEq(briVault.stakedAsset(msgSender), 0 ether);

assertEq(briVault.stakedAsset(receiver), stackedAsset);

// 3. check that shares are minted for msgSender after deposit

uint256 vaultBalanceBeforeCancelParticipation = mockToken.balanceOf(address(briVault));

uint256 vaultTotalSupplyBeforeCancelParticipation = briVault.totalSupply();

uint256 mintedShares =

Math.mulDiv(stackedAsset, vaultTotalSupplyBeforeCancelParticipation, vaultBalanceBeforeCancelParticipation);

assertEq(briVault.balanceOf(msgSender), mintedShares);

// 4. cancel participation by msgSender

briVault.cancelParticipation();

vm.stopPrank();

uint256 vaultBalanceAfterCancelParticipation = mockToken.balanceOf(address(briVault));

// 5. check that vault balance is not changed after 'cancelParticipation'

assertEq(vaultBalanceBeforeCancelParticipation, vaultBalanceAfterCancelParticipation);

// 6. check that all shares are burned from msg.sender and vault totalSupply decreased

assertEq(briVault.balanceOf(msgSender), 0);

assertEq(briVault.totalSupply(), (vaultTotalSupplyBeforeCancelParticipation - mintedShares));

/////////////////////////////////////////////////////////////////////////////////

// 'cancelParticipation' is called by address equal to 'receiver' from deposit //

/////////////////////////////////////////////////////////////////////////////////

address msgSender2 = user3;

address receiver2 = user4;

vm.prank(owner);

briVault2 = new BriVault(

IERC20(address(mockToken)),

participationFeeBsp,

eventStartDate,

participationFeeAddress,

minimumAmount,

eventEndDate

);

vm.startPrank(msgSender2);

mockToken.approve(address(briVault2), depositAmount);

// 1. deposit with msg.sender2 != receiver2

briVault2.deposit(depositAmount, receiver2);

vm.stopPrank();

// 2. check that deposited asset amount is stored for receiver2, not for msgSender2

assertEq(briVault2.stakedAsset(msgSender2), 0 ether);

assertEq(briVault2.stakedAsset(receiver2), stackedAsset);

// 3. check that shares are minted for msgSender2 after deposit

uint256 vaultBalanceBeforeCancelParticipation2 = mockToken.balanceOf(address(briVault2));

uint256 vaultTotalSupplyBeforeCancelParticipation2 = briVault2.totalSupply();

uint256 mintedShares2 = Math.mulDiv(

stackedAsset, vaultTotalSupplyBeforeCancelParticipation2, vaultBalanceBeforeCancelParticipation2

);

assertEq(briVault2.balanceOf(msgSender2), mintedShares2);

// 4. cancel participation by receiver2

vm.prank(receiver2);

briVault2.cancelParticipation();

uint256 vaultBalanceAfterCancelParticipation2 = mockToken.balanceOf(address(briVault2));

// 5. check that vault balance decreased after 'cancelParticipation'

assertEq(vaultBalanceBeforeCancelParticipation2 - stackedAsset, vaultBalanceAfterCancelParticipation2);

// 6. check that shares are not burned from msg.sender and vault totalSupply is not changed

assertEq(briVault2.balanceOf(msgSender2), mintedShares2);

assertEq(briVault2.totalSupply(), vaultTotalSupplyBeforeCancelParticipation2);

}

Recommended Mitigation

Shares amount to burn should always be calculated depending on the amount of participant's stackedAssets as specified in ERC4626 withdraw function.
Make the signature and logic of the cancelParticipation function clearer:

+function cancelParticipation(address receiver, address sharesOwner, address assetsOwner) public {

if (block.timestamp >= eventStartDate) {

revert eventStarted();

}

- stakedAsset[msg.sender] = 0;

+ stakedAsset[assetsOwner] = 0;

- uint256 shares = balanceOf(msg.sender);

+ uint256 shares = balanceOf(sharesOwner);

+ // calculate assets to withdraw depending on the current exchange rate:

+ uint256 assetToWithdraw = Math.mulDiv(shares, IERC20(asset()).balanceOf(address(this)), totalSupply());

+ // depending on desired behavior, do not refund more than deposited...

- uint256 refundAmount = stakedAsset[msg.sender];

+ uint256 refundAmount = stakedAsset[assetsOwner];;

+ if(refundAmount > assetToWithdraw){

+ refundAmount = assetToWithdraw;

+ }

_burn(msg.sender, shares);

- IERC20(asset()).safeTransfer(msg.sender, refundAmount);

+ IERC20(asset()).safeTransfer(receiver, refundAmount);

}

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

Rewards breakdown

nSLOC

193

This contest has a flat reward structure with the following payouts:

High

100 EXP

Medium

20 EXP

Low

2 EXP

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being carefully reviewed by our judges.

View all submissions

Appeals

This is your time to appeal against judgements on your submissions.

Appeals Review

Appeals are being carefully reviewed by our judges.

Rewards Distribution

The contest is complete and the rewards are being distributed.

View results

Support

FAQs

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

Give us feedback!