Incorrect ERC-20 deposit handling

• Target: state_transition.go

• Category: Coding Mistakes

• Severity: High

• Likelihood: N/A

• Impact: Informational

Description

The pull request under review rebases Astria preexisting changes on top of official Geth 1.14.3. At the time of the start of this engagement, PR #21 HEAD was at commit a30fd3d23b31dff26ded8abf373e0bc8050d08c7. During the first two days of our engagement, Astria independently found and fixed some issues (seemingly originating from incorrect manual merge-conflict resolutions), updating the PR HEAD to 5f9724be5ad41500855c9c6e6f76037e438f320c. This included a relatively severe issue in state_transition.go::TransitionDb.

The function was modified by Astria to handle the newly introduced deposit-transaction type. In the initial version of the code, the function code appeared as follows:

func (st *StateTransition) TransitionDb() (*ExecutionResult, error) {
    // if this is a deposit tx, we only need to mint funds and no gas is used.
    if st.msg.IsDepositTx {
        log.Debug("deposit tx minting funds", "to", *st.msg.To, "value", st.msg.Value)
        st.state.AddBalance(*st.msg.To, uint256.MustFromBig(st.msg.Value), tracing.BalanceIncreaseAstriaDepositTx)
        return &ExecutionResult{
            UsedGas:    0,
            Err:        nil,
            ReturnData: nil,
        }, nil
    }

    if st.msg.IsDepositTx {
        st.initialGas = st.msg.GasLimit
        st.gasRemaining = st.msg.GasLimit
        log.Debug("deposit tx minting erc20", "to", *st.msg.To, "value", st.msg.Value)
    }
    /// ...

The initial condition is intended to handle only native deposit transactions, and it was incorrect. ERC-20 deposits always have a msg.Value of zero but a nonempty msg.Data containing the calldata, which invokes the mint function of the destination contract that manages the bridged asset.

Impact

The TransitionDb function was incorrectly handling ERC-20 deposit transactions as native deposit transactions. This would have prevented successful bridging of ERC-20 assets, leading to the loss of the deposited assets.

Recommendations

None. (The issue was already independently found and fixed.)

Remediation

This issue has been acknowledged by Astria, and a fix was implemented in commit 5f9724be↗.

The issue was fixed as follows:

func (st *StateTransition) TransitionDb() (*ExecutionResult, error) {
    // if this is a deposit tx, we only need to mint funds and no gas is used.
-   if st.msg.IsDepositTx {
+   if st.msg.IsDepositTx && len(st.msg.Data) == 0 {
        log.Debug("deposit tx minting funds", "to", *st.msg.To, "value", st.msg.Value)
        st.state.AddBalance(*st.msg.To, uint256.MustFromBig(st.msg.Value), tracing.BalanceIncreaseAstriaDepositTx)
        return &ExecutionResult{
            UsedGas:    0,
            Err:        nil,
            ReturnData: nil,
        }, nil
    }

    if st.msg.IsDepositTx {
        st.initialGas = st.msg.GasLimit
        st.gasRemaining = st.msg.GasLimit
        log.Debug("deposit tx minting erc20", "to", *st.msg.To, "value", st.msg.Value)
    }
    /// ...