Chain4Travel Camino Node & Wallet Security Review Report

The AddressStateTx handler allows a user to remove the admin bit from themselves. If the only administrative account does this, there will be no remaining admins.

Disallow to remove the admin bit from the sender's account or if there are no other admins.

There is a revokeRole() function that can be called by an admin to remove roles from the account.

There is no check that the admin is not removing the admin role from himself. If the only administrative account does this, there will be no remaining admins.

Disallow to remove the admin bit from the sender's account or if there are no other admins.

Number of API endpoints of the node might be susceptible to DoS attacks. The denial of service may be caused by either a huge response size amplification or by heavy computation on the input data.

Examples of such API calls are platform.getCurrentValidators (proved to cause significant load and delays on the AVAX nodes) and platform.getClaimables (accepts an unbounded array of deposit tx ids).

Implement the default paging of the API.

An executor of the ImportTx transaction does not verify that tx.ImportedInputs does not have locked inputs via locked.VerifyNoLocks. Although it does not seem possible to export a locked UTXO from C-Chain at the moment of audit due to lack of locked.In and locked.Out definitions in C-Chain transaction codec, it might be possible to do so in future.

Add the following line in the ImportTx function in camino_tx_executor.go before executing transaction:

if err := locked.VerifyNoLocks(tx.ImportedInputs); err != nil { return err
}

The executor of RewardsImportTx does not verify credentials of transaction inputs which makes it possible for anyone to submit this transaction to the mempool. Although UTXO outputOwners can only be the treasury address, it might be worth investigating possible ways to authorize the submission of this transaction.

Review the rewards import procedure in order to make it possible to authorize the transactions coming from the mempool.

The function VerifyUnlockDepositedUTXOs verifies transaction credentials against inputs via VerifyMultisigTransfer only for non-deposit UTXOs.

if isDeposited = lockedOut.DepositTxID != ids.Empty; !isDeposited {
    return nil, errUnlockingUnlockedUTXO
}
/* … */
if isDeposited {
    /* this branch lacks VerifyMultisigTransfer */
} else {
    if err := h.fx.VerifyMultisigTransfer(tx, in, creds[index], out, h.utxosReader); err != nil {
        return nil, fmt.Errorf("failed to verify transfer: %w", err)
/* …*/
    }
}

Thus, it is possible to unlock an expired deposit for any account. Test case for camino_locked.go:

"UTXO outputOwners do not correspond to creds2": {
    args: args{
        chainState: func(ctrl *gomock.Controller) state.Chain {
            s := state.NewMockChain(ctrl)
            s.EXPECT().GetTimestamp().Return(depositExpiredTime)
            s.EXPECT().GetDeposit(depositID).Return(deposit1, nil)
            s.EXPECT().GetDepositOffer(deposit1.DepositOfferID).Return(depositOffer, nil)
            return s
        },
        tx: tx,
        utxos: []*avax.UTXO{
            generateTestUTXO(ids.ID{9, 9}, assetID, deposit1.Amount, outputOwners, depositID, ids.Empty),
        },
        ins: []*avax.TransferableInput{
            generateTestIn(assetID, deposit1.Amount, depositID, ids.Empty, sigIndices),
        },
        outs: []*avax.TransferableOutput{
            generateTestOut(assetID, deposit1.Amount, outputOwners, ids.Empty, ids.Empty),
        },
        creds:        []verify.Verifiable{cred2},
        burnedAmount: 0,
        assetID:      assetID,
    },
    want: map[ids.ID]uint64{depositID: deposit1.Amount},
},

Review the deposit unlock procedure in order to make it possible to authorize the transactions coming from the mempool.

Users can control the href attribute of the <a> tag in NFTs that contains links. This tag is supporting javascript: protocol scheme and can be used by attackers to perform tabnabbing or XSS attacks.

However the <a> tags have the target="_blank" attribute which provides the same rel behavior as setting rel="noopener". But if a user uses an old version of the browser users can be tricked by the attacker.

Don't render links with the javascript: schema and explicitly specify the rel="noopener noreferrer" attribute.

During the adding of the NFT tokens wallet is trying to read the symbol and name of the submitted contract via calling symbol() and name() functions. ERC1155 tokens don't have these getters and these calls are reverted.

Correctly implement the interface to the ERC1155 tokens in the wallet.

Avalanche wallet uses ERC721Enumerable to track current owners of an NFTs. Camino wallet collects owners of NFTs by parsing events. However, the wallet shows all tokens that were transferred to a user, even those that were already sent to another address and don't belong to the current user.

Correctly implement the NFT ownership detection.