Summary
The Minted event emits the ambiguous amount under certain conditions
Vulnerability Details
The PerpetualVault::_mint function will emit the Minted event with amount of indexToken or collateralToken. However, the emitted event not tracked the amount is indexToken or collateralToken which may lead to confusion and affect the traceability.
function _mint(uint256 depositId, uint256 amount, bool refundFee, MarketPrices memory prices) internal {
uint256 _shares;
if (totalShares == 0) {
_shares = depositInfo[depositId].amount * 1e8;
} else {
uint256 totalAmountBefore;
if (positionIsClosed == false && _isLongOneLeverage(beenLong)) {
totalAmountBefore = IERC20(indexToken).balanceOf(address(this)) - amount;
} else {
totalAmountBefore = _totalAmount(prices) - amount;
}
if (totalAmountBefore == 0) totalAmountBefore = 1;
_shares = amount * totalShares / totalAmountBefore;
}
depositInfo[depositId].shares = _shares;
totalShares = totalShares + _shares;
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[counter].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
@> emit Minted(depositId, depositInfo[depositId].owner, _shares, amount);
}
Impact
The emitted Minted event with ambiguous amount may lead to confusion and affect the traceability.
PoC
This PoC shown the issue with the Minted event across different position statuses: even when users deposit the same amount, the amount may be emitted in different tokens.
Copy the following test case to the test/PerpetualVault.t.sol file
Run tests with forge test --mt test_Minted_Event_With_Ambiguous_Amount --rpc-url arbitrum
function test_Minted_Event_With_Ambiguous_Amount() external {
address keeper = PerpetualVault(vault).keeper();
address alice = makeAddr("alice");
IERC20 collateralToken = PerpetualVault(vault).collateralToken();
uint256 executionFeeGasLimit = PerpetualVault(vault).getExecutionGasLimit(true);
uint256 executionFee = executionFeeGasLimit * tx.gasprice;
deal(alice, executionFee);
deal(address(collateralToken), alice, 1e10);
vm.startPrank(alice);
collateralToken.approve(vault, 1e10);
vm.expectEmit();
emit PerpetualVault.Minted(1, alice, 1000000000000000000, 10000000000);
PerpetualVault(vault).deposit{value: executionFee}(1e10);
vm.stopPrank();
MarketPrices memory prices = mockData.getMarketPrices();
bytes memory paraSwapData = mockData.getParaSwapData(vault);
bytes[] memory swapData = new bytes[](1);
swapData[0] = abi.encode(PROTOCOL.DEX, paraSwapData);
vm.prank(keeper);
PerpetualVault(vault).run(true, true, prices, swapData);
executionFeeGasLimit = PerpetualVault(vault).getExecutionGasLimit(true);
executionFee = executionFeeGasLimit * tx.gasprice;
deal(alice, executionFee);
deal(address(collateralToken), alice, 1e10);
vm.startPrank(alice);
collateralToken.approve(vault, 1e10);
PerpetualVault(vault).deposit{value: executionFee}(1e10);
vm.stopPrank();
vm.prank(keeper);
vm.expectEmit();
emit PerpetualVault.Minted(2, alice, 1000000000000000000, 2916781326862221805);
PerpetualVault(vault).runNextAction(prices, swapData);
}
Tools Used
Manual Review
Recommendations
Adding a new parameter to the Minted event to indicate the amount is in term of collateral token or index token. And then update the PerpetualVault::_mint function to emit the Minted event with the new parameter.
- event Minted(uint256 depositId, address depositor, uint256 shareAmount, uint256 depositAmount);
+ event Minted(uint256 depositId, address depositor, uint256 shareAmount, uint256 depositAmount, bool isIndexToken);
function _mint(uint256 depositId, uint256 amount, bool refundFee, MarketPrices memory prices, bool isCollateralToken) internal {
uint256 _shares;
if (totalShares == 0) {
_shares = depositInfo[depositId].amount * 1e8;
} else {
uint256 totalAmountBefore;
if (positionIsClosed == false && _isLongOneLeverage(beenLong)) {
totalAmountBefore = IERC20(indexToken).balanceOf(address(this)) - amount;
} else {
totalAmountBefore = _totalAmount(prices) - amount;
}
if (totalAmountBefore == 0) totalAmountBefore = 1;
_shares = amount * totalShares / totalAmountBefore;
}
depositInfo[depositId].shares = _shares;
totalShares = totalShares + _shares;
if (refundFee) {
uint256 usedFee = callbackGasLimit * tx.gasprice;
if (depositInfo[counter].executionFee > usedFee) {
try IGmxProxy(gmxProxy).refundExecutionFee(depositInfo[counter].owner, depositInfo[counter].executionFee - usedFee) {} catch {}
}
}
- emit Minted(depositId, depositInfo[depositId].owner, _shares, amount);
+ emit Minted(depositId, depositInfo[depositId].owner, _shares, amount, positionIsClosed == false && _isLongOneLeverage(beenLong));
}
