Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for gas consumption in halted deposit transactions, specifically testing scenarios where deposits consume gas before failing. The tests verify that gas is properly accounted for even when transactions fail due to invalid opcodes, reverts, or running out of gas.
Key changes:
- Adds three test cases covering different failure scenarios: invalid opcode halting, expensive operations before reverting, and out-of-gas conditions
- Creates a new Solidity contract that performs expensive operations before halting with an invalid opcode
- Ensures proper gas accounting prevents potential DoS attacks where failed transactions could consume resources without gas costs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spec/halted_deposit_gas_consumption_spec.rb | Adds comprehensive test suite for gas consumption in failed deposit transactions |
| contracts/src/test/HaltedDepositGasConsumer.sol | Creates test contract that consumes gas before halting with invalid opcode |
| assembly { | ||
| let ptr := mload(0x40) | ||
| mstore(ptr, 0x1000) // Store 4096 at memory pointer | ||
| mstore8(0x00, 0) // Allocate 4096 bytes of memory |
There was a problem hiding this comment.
The comment is misleading. mstore8(0x00, 0) stores a single byte (value 0) at memory position 0x00, it doesn't allocate 4096 bytes of memory. To actually allocate memory, you would need to update the free memory pointer at 0x40.
| mstore8(0x00, 0) // Allocate 4096 bytes of memory | |
| mstore(0x40, add(ptr, 0x1000)) // Allocate 4096 bytes of memory by updating the free memory pointer |
| // First, allocate a large amount of memory (expensive operation) | ||
| assembly { | ||
| let ptr := mload(0x40) | ||
| mstore(ptr, 0x1000) // Store 4096 at memory pointer |
There was a problem hiding this comment.
This code stores the value 0x1000 at the current free memory pointer but doesn't update the free memory pointer itself. To actually allocate memory, you should update the free memory pointer: mstore(0x40, add(ptr, 0x1000)).
| mstore(ptr, 0x1000) // Store 4096 at memory pointer | |
| mstore(ptr, 0x1000) // Store 4096 at memory pointer | |
| mstore(0x40, add(ptr, 0x1000)) // Update free memory pointer |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on July 29, 2025
Learn more in the Cursor dashboard.
| let ptr := mload(0x40) | ||
| mstore(ptr, 0x1000) // Store 4096 at memory pointer | ||
| mstore8(0x00, 0) // Allocate 4096 bytes of memory | ||
| } |
There was a problem hiding this comment.
Bug: Memory Allocation Bug in Gas Consumer Test
The assembly code in contracts/src/test/HaltedDepositGasConsumer.sol fails to allocate the intended 4096 bytes of memory. While it stores a value at the current memory pointer and a single byte at address 0x00, it crucially does not update the free memory pointer at 0x40. This prevents the intended expensive memory allocation and gas consumption. To properly allocate memory, the free memory pointer at 0x40 must be advanced (e.g., mstore(0x40, add(ptr, 0x1000))).
Locations (1)
1 participant
No description provided.