Elysium security record

The functions `Erc20DepositHandler`, `Erc721DepositHandler` and `GenericDepositHandler` have many byte manipulation operations which fetch encoded length and then read the bytes according to the length. However, some basic size checks are missing to ensure it would not go over the boundary. Missing checks on fetched 'length' guarantees it won't go beyond `calldata`'s length. Some (not all) examples are listed below: ```go=86 recipientAddressLength := big.NewInt(0).SetBytes(calldata[32:64]) // 64 - (64 + recipient address length) is recipient address recipientAddress := calldata[64:(64 + recipientAddressLength.Int64())] ``` ```go=117 metadataLen := big.NewInt(0).SetBytes(calldata[:32]) metadata := calldata[32 : 32+metadataLen.Int64()] ``` ```go=139 recipientAddressLength := big.NewInt(0).SetBytes(calldata[32:64]) // 64 - (64 + recipient address length) is recipient address recipientAddress := calldata[64:(64 + recipientAddressLength.Int64())] // (64 + recipient address length) - ((64 + recipient address length) + 32) is metadata length metadataLength := big.NewInt(0).SetBytes( calldata[(64 + recipientAddressLength.Int64()):((64 + recipientAddressLength.Int64()) + 32)], ) ```

File: chains/evm/calls/transactor/signAndSend/signAndSend.go In the function `Transact`, the workflow is that: 1, call t.client.SignAndSendTransaction to send the transaction to the blockchain node. 2, using the function `WaitAndReturnTxReceipt` to check the transaction result. In step 2, if the function `WaitAndReturnTxReceipt` returns err, it doesn't mean the transaction failed entirely. For example, some error could have occurred on the remote blockchain endpoint when calling the function WaitAndReturnTxReceipt. Thus a better way is to return the transaction hash to check the receipt later. ```golang=76 if err != nil { return &common.Hash{}, err } ```

There is a risk that a block could be processed twice across bridge reboot. At listener.go, as shown at line 81, the id of the successfully processed block would be persisted to local db. ```go=51 func (l *EVMListener) ListenToEvents(ctx context.Context, block *big.Int, msgChan chan *message.Message, errChn chan<- error) { for { select { case <-ctx.Done(): return default: head, err := l.client.LatestBlock() if err != nil { log.Error().Err(err).Msg("Unable to get latest block") time.Sleep(l.blockRetryInterval) continue } if block == nil { block = head } // Sleep if the difference is less than needed block confirmations; (latest - current) < BlockDelay if big.NewInt(0).Sub(head, block).Cmp(l.blockConfirmations) == -1 { time.Sleep(l.blockRetryInterval) continue } log.Debug().Msgf("Queried block in listner %s", block.String()) for _, handler := range l.eventHandlers { err := handler.HandleEvent(block, msgChan) if err != nil { log.Error().Err(err).Str("DomainID", string(l.domainID)).Msgf("Unable to handle events") continue } } //Write to block store. Not a critical operation, no need to retry err = l.blockstore.StoreBlock(block, l.domainID) if err != nil { log.Error().Str("block", block.String()).Err(err).Msg("Failed to write latest block to blockstore") } block.Add(block, big.NewInt(1)) } } } ``` After the bridge is rebooted, the previously persisted block id (this block's been successfully processed at the last run before the rebooting) would be read in and passed to the function `ListenToEvents` as the starting block, which will be processed again. ```go=59 func (bs *BlockStore) GetStartBlock(domainID uint8, startBlock *big.Int, latest bool, fresh bool) (*big.Int, error) { if latest { return nil, nil } if fresh { return startBlock, nil } latestBlock, err := bs.GetLastStoredBlock(domainID) if err != nil { return nil, err } if latestBlock.Cmp(startBlock) == 1 { return latestBlock, nil } else { return startBlock, nil } } ```

The variable `Dst` is repeatedly verified. `ValidateMintFlags` verifies the format of `Dst`, but later it is again verified by `ProcessMintFlags`. ```go=41 Args: func(cmd *cobra.Command, args []string) error { err := ValidateMintFlags(cmd, args) if err != nil { return err } err = ProcessMintFlags(cmd, args) return err }, ``` ```go=74 func ProcessMintFlags(cmd *cobra.Command, args []string) error { Erc721Addr = common.HexToAddress(Erc721Address) if !common.IsHexAddress(Dst) { DstAddress = senderKeyPair.CommonAddress() } else { DstAddress = common.HexToAddress(Dst) } ```

The interface `Transactor` defined at prepare.go is the same as the one defined at transact.go. ```go=36 type Transactor interface { Transact(to *common.Address, data []byte, opts TransactOptions) (*common.Hash, error) } ```

The command to remove relayers is disabled, which prevents excluding a potentially malicious or misbehaving relayer. The relayers are an essential part of the bridge since they listen and filter out the necessary messages from a domain chain to a target chain. They participate in the actual bridging process by creating a interchain transfer proposals or voting on them. Thus, it is important that the relayer set is kept robust, without a portion of them going rogue or going down, which can cause problems during the transfer process. Currently, `removeRelayer` function is commented out, disabling the functionality to remove a relayer from the active set. ```go=60 /* func removeRelayer(cctx *cli.Context) error { ... } */ ```

These handler functions directly return nil without real data query. The actual implementations were commented out. ```go=53 func cancelProposal(cmd *cobra.Command, args []string) error { log.Debug().Msgf(` Cancel propsal Bridge address: %s Chain ID: %d Deposit nonce: %d DataHash: %s `, Bridge, DomainID, DepositNonce, DataHash) return nil } ``` ```go=53 func queryProposal(cmd *cobra.Command, args []string) error { log.Debug().Msgf(` Querying proposal Chain ID: %d Deposit nonce: %d Data hash: %s Bridge address: %s`, DomainID, DepositNonce, DataHash, Bridge) return nil } ``` ```go=51 func queryResource(cmd *cobra.Command, args []string) error { log.Debug().Msgf(` Querying resource Handler address: %s Resource ID: %s`, Handler, ResourceID) return nil } ```

The array gasPrices can be directly initialized with the field value without having to create first. In Go, you can directly initialize any array with initial field values. Unless the array is too big to initialize with internal values through direct expressions, you're recommended to declare the fields inside when you're creating the array. The original code makes the array first and pushes a value. ```go=50 gasPrices := make([]*big.Int, 1) gasPrices[0] = gp ``` However it can be directly initialized with the value. ```go=50 gasPrices = []*big.Int{gp} ```

`repetitiveSimulateVote` uses an unnecessary recursion logic that can be removed. The function `repetitiveSimulateVote` uses a fixed maximum tries with a starting try count to repeatedly simulate a proposal vote until it succeeds. However, we can remove the global maximum try count and directly input a number into the beginning of the function and try that number of times until it succeeds, without using recursion or global fixed maximum retries. ```go=186 func (v *EVMVoter) repetitiveSimulateVote(prop *proposal.Proposal, tries int) error { err := v.bridgeContract.SimulateVoteProposal(prop) if err != nil { if tries < maxSimulateVoteChecks { tries++ return v.repetitiveSimulateVote(prop, tries) } return err } else { return nil } } ```

Variables `ABI` and 'Transactor` have no external calls, so they can be set to private. ```go=18 type Contract struct { contractAddress common.Address ABI abi.ABI bytecode []byte client calls.ContractCallerDispatcher transactor.Transactor } ```