Summary
Both afterDepositCancellation() and afterWithdrawalCancellation() could encounter a situation where if CachedKey != Key, the function will enter an else if condition but not execute any further operations because the nested if statement is not met.
Vulnerability Details
In this example, we can see that we have two nested conditions, and if one 'if' statement is met but the nested condition is not, the final 'else' will not execute. Consequently, no revert or state change will occur, and the function execution will end without an error or changes.
function afterDepositCancellation(
bytes32 depositKey, IDeposit.Props memory, IEvent.Props memory )
external
onlyController
{
GMXTypes.Store memory _store = vault.store();
if (_store.status == GMXTypes.Status.Deposit) {
if (_store.depositCache.depositKey == depositKey) {
vault.processDepositCancellation();
}
} else if (_store.status == GMXTypes.Status.Rebalance_Add) {
if (_store.rebalanceCache.depositKey == depositKey) {
vault.processRebalanceAddCancellation();
}
} else if (_store.status == GMXTypes.Status.Compound) {
if (_store.compoundCache.depositKey == depositKey) {
vault.processCompoundCancellation();
}
} else {
revert Errors.DepositCancellationCallback();
}
}
Same happen here:
function afterWithdrawalExecution(
bytes32 withdrawKey, IWithdrawal.Props memory, IEvent.Props memory )
external
onlyController
{
GMXTypes.Store memory _store = vault.store();
if (_store.status == GMXTypes.Status.Withdraw && _store.withdrawCache.withdrawKey == withdrawKey) {
vault.processWithdraw();
} else if (
_store.status == GMXTypes.Status.Rebalance_Remove && _store.rebalanceCache.withdrawKey == withdrawKey
) {
vault.processRebalanceRemove();
} else if (_store.status == GMXTypes.Status.Deposit_Failed && _store.depositCache.withdrawKey == withdrawKey) {
vault.processDepositFailureLiquidityWithdrawal();
}
}
If a test is conducted with a changed key, we will see the assertion checking for the vault's state fail. In this instance, it is not reopened.
Example:
[PASS] test_processDepositCancel() (gas: 1214503)
Logs:
deposit token address 0x6E1734aC57e76fcD7fD66266Ae0C2547dB3A713a
is native token?? true
is token A true
is token B false
borrows amount after added leverage + adjusments
borrowTokenAAmt 0
borrowTokenBAmt 3200000000
msg.sender in deposti function 0xdd845642a112D7cBd82EFE83619EB39f0894521B
msg.sender 0xdd845642a112D7cBd82EFE83619EB39f0894521B
VAULT STATUS 0
[FAIL. Reason: NotAllowedInCurrentVaultStatus()] test_processDepositFailure() (gas: 1465094)
Logs:
vault address 0x6dA36424D631fcc6fC8CC621B6d6d2A087D7CF17
value should be 0 and is: 1000000000000000
deposit token address 0x6E1734aC57e76fcD7fD66266Ae0C2547dB3A713a
is native token?? false
is token A true
is token B false
borrows amount after added leverage + adjusments
borrowTokenAAmt 0
borrowTokenBAmt 3200000000
msg.sender in deposti function 0xdd845642a112D7cBd82EFE83619EB39f0894521B
msg.sender 0xdd845642a112D7cBd82EFE83619EB39f0894521B
Error: a == b not satisfied [uint]
Expected: 2
Actual: 1
Impact
The state of the vault will remain unchanged and the function will not revert considering this like a successful tx. Keepers will be not aware of that because the state is not changed and no revert is done. This cause that keepers/sentinel can not detect this and a human action will be required to reset the state back to Open.
Tools Used
Manual Review
Recommendations
Use the following pattern checking both key and status with and && and not nested if's:
function afterDepositExecution(
bytes32 depositKey,
IDeposit.Props memory,
IEvent.Props memory
) external onlyController {
GMXTypes.Store memory _store = vault.store();
if (
_store.status == GMXTypes.Status.Deposit
&& _store.depositCache.depositKey == depositKey
) {
vault.processDeposit();
} else if (_store.status == GMXTypes.Status.Rebalance_Add && _store.rebalanceCache.depositKey == depositKey) {
vault.processRebalanceAdd();
} else if (_store.status == GMXTypes.Status.Compound && _store.compoundCache.depositKey == depositKey) {
vault.processCompound();
} else if (_store.status == GMXTypes.Status.Withdraw_Failed && _store.withdrawCache.depositKey == depositKey) {
vault.processWithdrawFailureLiquidityAdded();
} else if (_store.status == GMXTypes.Status.Resume) {
vault.processEmergencyResume();
}
}
