Fix the vulnerability found in issue #3861

[Lexterl33t]

Fix the vulnerability found in issue #3861

Description

This PR addresses the non-compliance issue with the PUSHN opcodes in EthereumJS, as described in issue #3861.

Previously, when a PUSH instruction had fewer bytes available than required, the missing bytes were not properly zero-padded to align with the Ethereum Yellow Paper specification.

This led to inconsistencies between EthereumJS and other EVM implementations like REVM and SputnikVM, which correctly apply right-aligned zero-padding.

---

Fix Implemented

• Ensured that PUSHN opcodes always zero-pad missing bytes, right-aligning the values as per the Yellow Paper.
• Added a test case to validate that the stack correctly returns the padded value.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.51%. Comparing base (1774df6) to head (707c90f).
Report is 1 commits behind head on master.

Additional details and impacted files

Flag	Coverage Δ
block	76.87% <ø> (ø)
blockchain	85.69% <ø> (ø)
client	66.26% <ø> (ø)
common	90.31% <ø> (ø)
devp2p	76.21% <ø> (-0.07%)	⬇️
ethash	81.04% <ø> (ø)
evm	69.39% <100.00%> (+0.04%)	⬆️
genesis	99.84% <ø> (ø)
mpt	59.92% <ø> (-0.21%)	⬇️
rlp	69.70% <ø> (ø)
statemanager	70.15% <ø> (ø)
tx	80.52% <ø> (ø)
util	85.16% <ø> (ø)
vm	57.81% <ø> (ø)
wallet	83.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[jochem-brouwer]

Some initial comments! 😄 👍

[jochem-brouwer]

LGTM, thanks, will leave this open for one more review. Have changed the code to run minimal extra logic per PUSH (only a length check) and in the edge case of the PUSH being at the end of the code, then setLengthRight will be called on the bytes. Have also expanded the tests to test all PUSH opcodes PUSH1-PUSH32 for this case.

Wait, never mind, this does not fix the case if jump analysis has been done:

      if (!runState.shouldDoJumpAnalysis) {
        runState.stack.push(runState.cachedPushes[runState.programCounter])
        runState.programCounter += numToPush
// This case is still wrong
      } else {
        let loadedBytes = runState.code.subarray(
          runState.programCounter,
          runState.programCounter + numToPush,
        )
        if (loadedBytes.length < numToPush) {
          loadedBytes = setLengthRight(loadedBytes, numToPush)
        }

        runState.programCounter += numToPush
        runState.stack.push(bytesToBigInt(loadedBytes))
      }

[jochem-brouwer]

Ok, interesting, I just realized that the original code only fixes in the case if there is no JUMPDEST analysis being done. In practice / live networks, this is always done: it is necessary to find if the target we are JUMPing to is a valid JUMPDEST (and not a byte which looks like JUMPDEST but which is actually part of a PUSH opcode). So once a contract hits their first (succesful) JUMP we have to do the analysis. This is a more complex test case than the simple test case of "just" the code of truncated PUSH opcodes, but it would be interesting to see if the fuzzer would also find this, more complex case :)

[acolytec3]

LGTM