Assessment reportsPublic findings
Back to Zellic site
↗
Assessment reports>Initia>High findings>Withdrawal hash clash using variable-length fields
GeneralOverview
Findings
Critical (3)
High (12)Missing gas charge on memo in native_nft_transferNext zapping ID potentially duplicatedPublished module names do not necessarily match binary moduleStablepools can be created with one or no assetsStablepool swap can be called, repeating the same assetFrozen module coin store can cause chain haltMalicious proposer can skip fee checkMove coins can be burned twiceQuery gas limit not enforced through bank moduleIncorrect signer check for shorthand accountsValidator set updates can skip current validatorsWithdrawal hash clash using variable-length fields
Medium (4)
Low (9)
Informational (3)
DiscussionGeneral DiscussionImprove AMM testsBridge executor minting powerCharge before using valuesEmpty allowed_publishers array allows every address to publishERC20Keeper with custom contractsSuggestions for additional security checksTable handle overflow leads to abort
Threat ModelWhat are threat models?stdlib
Audit ResultsSummary
AppendixProof of concept: Reference safety verifier bypass
Category: Coding Mistakes

Withdrawal hash clash using variable-length fields

High Severity
Medium Impact
Medium Likelihood

Description

When finalizing a token withdrawal, a hash is generated using the provided values to ensure the same withdrawal is not processed twice and that it can be used to generate the root hash:

// verify storage root can be generated from
// withdrawal proofs and withdrawal tx data.
{
    var withdrawalHash [32]byte
    {
        seed := []byte{}
        seed = binary.BigEndian.AppendUint64(seed, bridgeId)
        seed = binary.BigEndian.AppendUint64(seed, req.Sequence)
        seed = append(seed, sender...)
        seed = append(seed, receiver...)
        seed = append(seed, []byte(denom)...)
        seed = binary.BigEndian.AppendUint64(seed, amount.Uint64())

        withdrawalHash = sha3.Sum256(seed)
    }

    if ok, err := ms.HasProvenWithdrawal(ctx, bridgeId, withdrawalHash); err != nil {
        return nil, err
    } else if ok {
        return nil, types.ErrWithdrawalAlreadyFinalized
    }

    // should works with sorted merkle tree
    rootSeed := withdrawalHash
    proofs := req.WithdrawalProofs
    for _, proof := range proofs {
        switch bytes.Compare(rootSeed[:], proof) {
        case 0, 1: // equal or greater
            rootSeed = sha3.Sum256(append(proof, rootSeed[:]...))
        case -1: // less
            rootSeed = sha3.Sum256(append(rootSeed[:], proof...))
        }
    }

    rootHash := rootSeed
    if !bytes.Equal(req.StorageRoot, rootHash[:]) {
        return nil, types.ErrFailedToVerifyWithdrawal.Wrap(

The issue is that the receiver and denom are both variable lengths; it is possible to generate the same hash by shifting bytes from the end of one to the start of another or vice versa. This could allow someone to effectively burn coins being held by the bridge under certain circumstances.

Impact

With the way Initia is currently set up, it would require an L1 denom that can be represented in hex, as the only denoms that an attacker can create are in the format move/[hex_address]. However, if another chain were to use the OPInit stack or if a new L1 token were added to the L1, it could open this up for attack.

Recommendations

A separator should be added between the variable-length fields to ensure that they cannot be shifted to generate the same hash.

Remediation

Zellic © 2024Back to top ↑