Liquidity Management
Gamma
DeFiFoundry
50,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Execution fee refunds will never be received if more than 2300 gas is consumed by the recipient

hgrano

Summary

When the execution fees are refunded the Solidity transfer function is used which uses a fixed stipend of 2300 gas. If this is insufficient then this error is caught and ignored, and consequently the refund will never be received.

Vulnerability Details

On GmxProxy:343, the owner of the deposit is sent back the refund amount using transfer. It is possible this might revert due to insufficient gas. Then in PerpetualVault (lines 783, 1148, 1226, 1232) this error is caught and ignored.

To verify this, you can add the following contract to PerpetualVault.t.sol:

contract TestRecipient {
mapping(uint256 => uint256) data;
uint256 counter;
address vault;
​
constructor(address _vault) {
vault = _vault;
counter = 1;
PerpetualVault(vault).collateralToken().approve(vault, type(uint256).max);
}
​
function deposit(uint256 amount) external payable {
PerpetualVault(vault).deposit{value: msg.value}(amount);
}
​
receive() external payable {
data[counter++] = counter;
}
}

Also add the below test case to PerpetualVaultTest and run using

forge test --mp test/PerpetualVault.t.sol --match-test test_failedRefund --via-ir --rpc-url arbitrum -vvv .

function test_failedRefund() external {
address keeper = PerpetualVault(vault).keeper();
​
// Setup the test contract which is designed to receive the refund
TestRecipient recipient = new TestRecipient(vault);
deal(address(PerpetualVault(vault).collateralToken()), address(recipient), 1 ether);
​
// Make an initial deposit
recipient.deposit(1e10);
​
// Do the initial swap
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);
​
// Deposit again with non-zero value
uint256 depositValue = 1 ether;
recipient.deposit{value: depositValue}(1e10);
​
// Cancel flow and the refund should be sent back
vm.prank(keeper);
PerpetualVault(vault).cancelFlow();
​
// The below assertion fails as the recipient balance is zero
assertEq(address(recipient).balance, depositValue, "Refund should be received");
}

Impact

If a contract is used to deposit or withdraw then it will not receive the required refund if it uses more than 2300 gas.

Tools Used

Foundry

Recommendations

Consider adding refund amounts into a mapping rather than returning directly to the deposit owners. Create a separate function through which deposit owners can claim the refunds themselves and delete them from the mapping.

Updates

Lead Judging Commences

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

Informational or Gas

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Users mistake, only impacting themselves.

Please read the CodeHawks documentation to know which submissions are valid. If you disagree, provide a coded PoC and explain the real likelihood and the detailed impact on the mainnet without any supposition (if, it could, etc) to prove your point.

Support

FAQs

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

Give us feedback!