State Guadian Network v2 Security Review

# State Guadian Network v2 Security Review ## Summary The Celer team requested to review the security of their newly developed State Guardian Network version 2 (from this point referred to as SGNv2), an application-specific blockchain based on the Cosmos SDK. The following chapters contain the results of a three-week security review that took place between the 21st of February and the 11th of March 2022. ### Reviewer This security review was performed by thec00n see [1](https://github.com/thec00n) and [2](https://twitter.com/g3rh4rdw4gn3r). ### Scope Reviewing the codebase of the SGNv2 is the primary objective of the engagement. The respective repository at [sgnv2](https://github.com/celer-network/sgnv2) is in scope and code changes up to commit hash [f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705](https://github.com/celer-network/sgnv2/commit/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705) are considered for the security review. The smart contracts of the SGNv2 [sgn-v2-contracts](https://github.com/celer-network/sgn-v2-contracts) are not in scope of the security review but will be selectively reviewed to understand the security properties of the system. The closed source SGNv2 gateway component are also not in scope. ### Regression testing The Celer team requested regression testing to be conducted in early April 2023 based on commit [b4d11dc28b40f4899416cf3f6762ad872ef1b597](https://github.com/celer-network/sgnv2/commit/b4d11dc28b40f4899416cf3f6762ad872ef1b597). **It has to be noted that the initial audit has occurred over one year ago and the code base has changed significantly since**. A best effort was made to verify that the vulnerabilities initially discovered were fixed adequately. ### Technical overview The figure below shows the overview of the SGNv2, including the bridge system. It mainly consists of the Ethereum staking and multi-chain bridge contracts and the validators that function as the SGNv2 network nodes. <img src="https://hackmd.io/_uploads/S1o7-0H-q.png" /> The roles and responsibilities of each component are summarized below: - Staking contracts host the CELR staking process. They specify and enforce some fundamental rules and configurations of roles, rewards, and penalties for SGNv2 validators and delegators. - Delegators stake their CELR tokens on the staking contract to elect validators. Delegators receive shares of SGNv2 block rewards and service fees proportional to their stakes. - The delegators elect validators on the staking contract. They run the SGNv2 chain to offer bridge services and calculate reward distributions. Validators receive commissions from their delegators. - Bridge contracts include the token bridge (both pool-based and peg-based) and message passing contracts that are deployed on multiple chains, which are used for cross-chain token transfers and message passing. - Users are the SGNv2 users, and they include senders of cross-chain token and message transfers or pool-based bridge liquidity providers. ### Key recommendations and observations The SGNv2 and its bridge contracts have a fairly complex threat model considering its heterogeneous nature and its dependency on other blockchain systems. The following list contains several observations and recommendations to understand the current gaps and to improve the system's resilience long term. | <div style="width:290px">Observation</div> | Recommendation | | ------------- |------------- | | The security review was conducted in a time box approach. As the SGNv2 codebase contains more than 200k LOC, it was only possible to review the most important parts of the system. | Additional security reviews of the codebase should be conducted. Furthermore, the SGNv2 should be included in the current bug bounty program ahead of the production launch. | | All findings have been labeled with a severity rating and listed in chapter [Detailed Findings](#Detailed-Findings). | All findings should be fixed, and the code changes should be linked to the respective finding. | | There are currently 23 chains listed in the SGNv2 production [configuration](https://github.com/celer-network/sgn-v2-networks/blob/main/sgn-3/cbridge.toml). If one of those chains is compromised, this could have severe economic consequences for the network and its liquidity pools. | A risk assessment of all connected chains should be conducted to make sure that at least the economic requirements outweigh the potential gains from a potential attack. The risk parameters for each connected chain should be monitored carefully and reassessed in periodic intervals. For instance, the Optimism chain currently has a block delay of one block which means that the SGNv2 almost immediately processes emitted events. This setting creates implicit trust in the operator of this chain as it assumes that it behaves as expected and is not compromised. Also, once fraud proofs are activated in Optimism, it is necessary to update block delay since anyone could submit an incorrect block, and challenging it might take some time. This example should highlight the necessity of revising the risk parameters of all connected chains. | | The CBR price updates are pushed to an AWS storage location in a periodic interval. All validators of the network download the asset prices from this location. This centralized price feed could cause severe damage if it is ever compromised or malfunctions. | It is strongly recommended to decentralize the asset price feed. | | At the time of the security review, only end-to-end tests exist in the codebase that test the happy path. Negative test cases to check if application-level controls are working are completely missing. Also, unit tests, in general, are none existent. The current level of testing coverage is a major concern as the SGNv2 is further developed and updated.| Additional tests should be created that specifically test message verification and processing endpoints. Negative test scenarios should be checked for all endpoints to verify that the application controls are working. | | The documentation is somewhat outdated. The protocol specifications have gaps but are of sufficient quality for the most part. | The appendix chapter [Update documentation](#Update-documentation) contains some recommendations on updating and improving the documentation. | | There are gaps in authorizing message processing endpoints that unnecessarily increase the attack surface of the system. | Appendix chapter [Restrict messages](#Restrict-messages) contains a list with all endpoints and recommendations on where stricter permissions should be applied. | | Validators who run an instance of SGNv2 are required to listen to events on all connected chains. | Create security best practices for validators that advise them on setting up full nodes and securing them. | ### Findings list Findings overview with a chapter link, finding severity and status is listed in the table below. For the full technical details, visit the [Detailed Findings](#Detailed-Findings) chapter. | Finding | Severity | Status | | ------------- |------------- |------------- | | [1. Send unauthorized messages via MessageBus](#1-Send-unauthorized-messages-via-MessageBus) | **Critical** |Fixed | | [2. Unverified parameter maxSlippage in cross-chain transfer](#2-Unverified-parameter-maxSlippage-in-cross-chain-transfer) | **High** |Fixed | | [3. State update verification does not consistently check for block delay](#3-State-update-verification-does-not-consistently-check-for-block-delay) | **High** |Partially Fixed | | [4. Unverified withdraw parameters are processed from bridge pool](#4-Unverified-withdrawal-parameters-are-processed-from-bridge-pool) | **Medium** |Fixed | | [5. None-validators can spam MsgProposeUpdates](5-None-validators-can-spam-MsgProposeUpdates) | **Medium** |Acknowledged | | [6. Set reward withdrawal address for delegators](#6-Set-reward-withdrawal-address-for-delegators) | **Medium** |Fixed | | [7. Same events can be submitted multiple times](#7-Same-events-can-be-submitted-multiple-times) | **Low** |Acknowledged | | [8. Pegbridge verification functions do not consistently check logs](#8-Pegbridge-verification-functions-do-not-consistently-check-logs) | **Low** |Fixed | | [9. Missing signature length check in RecoverSigner](#9-Missing-signature-length-check-in-RecoverSigner) | **Low** |Fixed | ## Detailed Findings ### 1. Send unauthorized messages via MessageBus | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **Critical** | Fixed| [evcmp.go ](https://github.com/celer-network/sgnv2/blob/b4d11dc28b40f4899416cf3f6762ad872ef1b597/eth/evcmp.go) | The function [DeepEqual](https://pkg.go.dev/reflect#DeepEqual) from package reflect is used to verify that the event struct fetched from chain strictly equals the struct that is submitted to the processing functions. | #### Description The `MessageBus` allows Dapps to send messages from one connected chain to another through the [MessageBusSender.sendMessage()](https://github.com/celer-network/sgn-v2-contracts/blob/ff5cffa6dd3abf940875c2a5dbdcadc64dee4732/contracts/message/messagebus/MessageBusSender.sol#L38-L46) contract function. The Relayer listens for the `Message` event that is emitted by this function, and `verifyMessage` is called to check its validity. Various properties are checked, and the function `verifyEventLog` on line 79 fetches the event directly from the emitting chain based on its transaction hash. On line 90, the `Equal` function is called which compares all attributes of the pending update log from the function call with the one that has been fetched on-chain. ```go=67 func (c *CbrOneChain) verifyMessage(cliCtx client.Context, eLog *ethtypes.Log, logmsg string) (done, approve bool) { // parse event ev, err := c.msgContract.ParseMessage(*eLog) if err != nil { log.Errorf("%s. parse eLog error %s", logmsg, err) return true, false } logmsg = fmt.Sprintf("%s. %s", logmsg, ev.String()) // check in store // check on chain done, approve, msgLog := c.verifyEventLog(eLog, eth.MsgBridge, msgbrtypes.MsgEventMessage, c.msgContract.GetAddr(), logmsg) if msgLog == nil { return done, approve } msgEv, err := c.msgContract.ParseMessage(*msgLog) if err != nil { log.Errorln(logmsg, "parse log err:", err) return true, false } // now cmp ev and msgEv if !ev.Equal(msgEv) { log.Errorln(logmsg, "ev not equal. got:", msgEv.String(), "expect:", ev.String()) return true, false } log.Infof("%s, success", logmsg) return true, true } ``` Verification takes place for every attribute by comparing the values against each other. If they do not match, the function returns with an error, and the pending update with the submitted event is not processed. Line 231 is the exception, as it does not compare the `Message` value but only its length. ```go=221 func (ev *MessageBusMessage) Equal(b *MessageBusMessage) bool { if ev.Sender != b.Sender { return false } if ev.DstChainId.Cmp(b.DstChainId) != 0 { return false } if ev.Receiver != b.Receiver { return false } if len(ev.Message) != len(b.Message) { return false } if ev.Fee.Cmp(b.Fee) != 0 { return false } return true } ``` [eth/utils.go#L221-L238](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/eth/utils.go#L221-L238) The insufficient check allows a malicious user to submit pending updates with logs that have already been processed with a different `Message` as long as the message length is equal to the event that actually occurred. Subsequent checks do not capture the already processed event. The `Message`, in the code sample below copied to `m.Data`, is used among other values from the event to generate the message id in `ComputeMessageIdNoTransfer`. This UID is used to determine a message's status and reject any events that have already been submitted. ```go=72 func (m *Message) ComputeMessageIdNoTransfer() []byte { data := solsha3.Pack( []string{"uint8", "address", "address", "uint64"}, []interface{}{uint8(MsgType_MSG_TYPE_MESSAGE), m.Sender, m.Receiver, m.SrcChainId}, ) // NOTE: Manual concatenation as solsha3 DOES NOT SUPPORT dynamic "bytes" data = append(data, m.Data...) hash := sha3.NewLegacyKeccak256() hash.Write(data) return hash.Sum(nil) } ``` A similar issue exists in `MessageBusMessageWithTransfer` on line 256 (see below code sample). `Message`'s length is compared, not the actual value. A malicious user could perform a similar attack and reuse an event that occurred but alter its `Message`. ```go=240 func (ev *MessageBusMessageWithTransfer) Equal(b *MessageBusMessageWithTransfer) bool { if ev.Bridge != b.Bridge { return false } if ev.SrcTransferId != b.SrcTransferId { return false } if ev.Sender != b.Sender { return false } if ev.DstChainId.Cmp(b.DstChainId) != 0 { return false } if ev.Receiver != b.Receiver { return false } if len(ev.Message) != len(b.Message) { return false } if ev.Fee.Cmp(b.Fee) != 0 { return false } return true } ``` [eth/utils.go#L240-L263](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/eth/utils.go#L240-L263) #### Impact A malicious user can resend arbitrary messages to connected chains from users that have submitted cross chain messages before without their authorization. Dapps could process a deposit of tokens multiple times even though it only occurred once. #### Remediation Compare the `Message` of `MessageBusMessage` and `MessageBusMessageWithTransfer` byte for byte between the submitted pending update event and the event retrieved from the source chain. ### 2. Unverified parameter `maxSlippage` in cross-chain transfer | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **High** |Fixed | [evcmp.go ](https://github.com/celer-network/sgnv2/blob/b4d11dc28b40f4899416cf3f6762ad872ef1b597/eth/evcmp.go) | `verifyRelay()` and `verifySend()` both check the `maxSlippage` parameter along with all other elements of the `BridgeRelay` and `BridgeSend` structs | #### Description Users initiate a cross chain transfer through the contract function [Bridge.send()](https://github.com/celer-network/sgn-v2-contracts/blob/ff5cffa6dd3abf940875c2a5dbdcadc64dee4732/contracts/Bridge.sol#L56-L67). As part of sending assets, they can set a `maxSlippage` parameter to make sure that they receive a minimum amount at the destination chain. The relayer listens for emitted `Send` events and submits them for verification, which is done by calculating a hash (referred to as `XferId`) with the parameters from the `Send` event and comparing the hash to the one on-chain. The `maxSlippage` parameter is not included in this check, as seen in the code snippet below. ```go=146 func (ev *BridgeSend) CalcXferId(srcChid uint64) Hash { var b []byte b = append(b, ev.Sender[:]...) b = append(b, ev.Receiver[:]...) b = append(b, ev.Token[:]...) b = append(b, ToPadBytes(ev.Amount)...) b = append(b, ToPadBytes(ev.DstChainId)...) b = append(b, ToPadBytes(ev.Nonce)...) // old contract uses uint256, new contract uses uint64 // b = append(b, ToPadBytes(big.NewInt(int64(srcChid)))...) b = append(b, ToPadBytes(srcChid)...) return Bytes2Hash(crypto.Keccak256(b)) } ``` [eth/utils.go#L146-L158](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/eth/utils.go#L146-L158) #### Impact A malicious user could pick up an event from a cross-chain transfer and submit a pending update to the validators with an arbitrary `maxSlippage` value before the valid one is processed. The transfer could lead to undesired losses from users who perform cross-chain transfers. #### Remediation The `maxSlippage` value from the pending update should be compared directly to the `Send` event since `XferId` does not include it. ### 3. State update verification does not consistently check for block delay | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **High** |Partially Fixed | [cbr_verifier.go](https://github.com/celer-network/sgnv2/blob/b4d11dc28b40f4899416cf3f6762ad872ef1b597/relayer/cbr_verifier.go), [pegbr_verifier.go](https://github.com/celer-network/sgnv2/blob/b4d11dc28b40f4899416cf3f6762ad872ef1b597/relayer/pegbr_verifier.go), [msgbr_verifier.go](https://github.com/celer-network/sgnv2/blob/b4d11dc28b40f4899416cf3f6762ad872ef1b597/relayer/msgbr_verifier.go)| `verifySend`, `verifyRelay`, `verifyWithdraw` and `verifyWithdrawalRequest` use the `verifyEventLog` function which checks the block delay for a specific chain (see also https://github.com/celer-network/sgn-v2-networks/blob/main/sgn-3/cbridge.toml) The other functions listed in the Description paragraph are not fixed. | #### Description The SGNv2 connects to various EVM-based chains by deploying smart contracts that track user actions and that emit events when a successful action has been taken place. It is crucial that the SGNv2 only processes verified events after a certain number of blocks. State changes could get recognized by the SGNv2 on a connected blockchain that might not end up being prolonged. Uncle blocks could occur by chance, or it could be due to a targeted attack. A malicious user could mine blocks and not publish them while at the same time sending transactions that register a large liquidity deposit. Once the validators recognize the deposit event, the held back blocks could be published. If they formed a longer chain than the blocks that included the large liquidity deposit, the malicious user would have performed a successful double-spend attack. There are controls in place to make a successful attack more difficult. State changes are only processed after a certain number of blocks (see the [production configuration](https://github.com/celer-network/sgn-v2-networks/blob/main/sgn-3/cbridge.toml)), but the delay is not consistently applied for all processed state changes. Some are processed as soon as the validators recognize them on a connected chain. The following list contains verification functions that do not include a block delay. * [verifyValidatorSgnAddr](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/verifier.go#L145-L175): Uses calls on the latest block * [verifyValidatorParams](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/verifier.go#L177-L219): Uses calls on the latest block * [verifyValidatorStates](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/verifier.go#L221-L260): Uses calls on the latest block * [verifyDelegatorShares](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/verifier.go#L262-L300): Uses calls on the latest block * [verifySend](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/cbr_verifier.go#L148-L198): Uses calls and considers the block delay. It is still recommended to process events rather than making calls. * [verifyRelay](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/cbr_verifier.go#L203-L238): Uses calls on the latest block * [verifyWithdraw](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/cbr_verifier.go#L240-L275): Uses calls on the latest block * [verifySigners](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/cbr_verifier.go#L319-L353): Uses calls on the latest block * [verifyWithdrawalRequest](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/cbr_verifier.go#L370-L398): Uses calls on the latest block #### Impact A malicious user needs to perform a reorg attack with at least one block, potentially more depending on how fast the SGNv2 recognizes the events at a specific chain and votes that the state changes took place. In case of a successful attack, a malicious user could: - become the sole validator of the SGNv2 without owning any CELR tokens. - withdraw funds from the liquidity pools with no previous deposit. - send funds from some source chain and receive them at some destination blockchain without actually sending them. #### Remediation All event verification functions should recognize updates only after the block delay. ### 4. Unverified withdrawal parameters are processed from bridge pool | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **Medium** |Fixed | [evcmp.go#L22-L25](https://github.com/celer-network/sgnv2/blob/b4d11dc28b40f4899416cf3f6762ad872ef1b597/eth/evcmp.go#L22-L25) | `verifyWithdraw` checks that all attributes of the submitted `BridgeWithdrawDone` struct match with the event fetched from the chain | #### Description The Pool contract emits the `WithdrawDone` event when the [withdraw()](https://github.com/celer-network/sgn-v2-contracts/blob/ff5cffa6dd3abf940875c2a5dbdcadc64dee4732/contracts/Pool.sol#L86-L110) function is called. The event has the following attributes. ```solidity= event WithdrawDone( bytes32 withdrawId, uint64 seqnum, address receiver, address token, uint256 amount, bytes32 refid ); ``` The relayer listens for emitted events and submits them for verification, which is done by calculating a hash (referred to as `WdID`) with the parameters from the `WithdrawDone` event and comparing hash to the one on-chain. The `refid` and `withdrawId` parameters are not included in this check, as seen in the code snippet below. ```go=177 func (ev *BridgeWithdrawDone) CalcWdID(chid uint64) Hash { var b []byte b = append(b, ToPadBytes(chid)...) b = append(b, ToPadBytes(ev.Seqnum)...) b = append(b, ev.Receiver[:]...) b = append(b, ev.Token[:]...) b = append(b, ToPadBytes(ev.Amount)...) return Bytes2Hash(crypto.Keccak256(b)) } ``` [eth/utils.go#L177-L185](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/eth/utils.go#L177-L185) #### Impact A malicious user could monitor for withdrawals and submit a pending update to the validators with an arbitrary `refid` and `withdrawId` value. The unchecked parameters could lead to integrity issues when `WithdrawDetail` is stored in state. It could be, for example, possible that an instance of `WithdrawDetail` of another user is overwritten. #### Remediation The `refid` and `withdrawId` values from the pending update should be compared directly to the `WithdrawDone` event retrieved on-chain since `WdID` does not include it. ### 5. None-validators can spam MsgProposeUpdates | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **Medium** |Acknowledged | | _Reponse from the team: Users do not have account in the SGN tendermint chains, so then cannot send spam updates. Unbounded validators are able to send spam events though. Given that current validators are whitelisted through the community governance, we consider the spam risk is low. We will add recommended check in later upgrades._ | #### Description Validators can relay on-chain event information to the network by proposing updates with `handleMsgProposeUpdates`. This function does not verify if the `msg.Sender` is a bonded validator and sends all submitted updates to the keeper. ```go=36 func handleMsgProposeUpdates( ctx sdk.Context, keeper keeper.Keeper, msg *types.MsgProposeUpdates, logEntry *seal.MsgLog) (*sdk.Result, error) { logEntry.Type = msg.Type() logEntry.Sender = msg.Sender err := keeper.ProposeUpdates(ctx, msg.Updates, msg.Sender, logEntry) if err != nil { return nil, err } return &sdk.Result{}, nil } ``` [sync/handler.go#L36-L47](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/x/sync/handler.go#L36-L47) #### Impact Users can spam `MsgProposeUpdates` with fake events. All other validators need to verify these events, which can put a considerable load on the network. The cost of such an attack will depend on the future gas prices on the SGNv2 production network. #### Remediation Only bonded validators should be able to call `handleMsgProposeUpdates`. Further, it is recommended to also restrict calls to `handleMsgVoteUpdates` as none-validators do not need to call this function. ### 6. Set reward withdrawal address for delegators | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **Medium** |Fixed | [4de4230966a45dc271297b4dd344529405a4a29](https://github.com/celer-network/sgn-v2/commit/4de4230966a45dc271297b4dd344529405a4a291) | _Response from the team: This was removed in a later release._ | #### Description The `SetWithdrawAddress` endpoint expects a `MsgSetWithdrawAddress` as input. It allows delegators to set an alternative withdrawal address. ```go=35 type MsgSetWithdrawAddress struct { // delegator_address defines the delegator's Ethereum address. DelegatorAddress string `protobuf:"bytes,1,opt,name=delegator_address,json=delegatorAddress,proto3" json:"delegator_address,omitempty" yaml:"delegator_address"` // validator_address defines the validator's Ethereum address. WithdrawAddress string `protobuf:"bytes,2,opt,name=withdraw_address,json=withdrawAddress,proto3" json:"withdraw_address,omitempty" yaml:"withdraw_address"` // sender defines the SGN account sending the claim Msg. Sender string `protobuf:"bytes,3,opt,name=sender,proto3" json:"sender,omitempty" yaml:"sender"` } ``` [distribution/types/tx.pb.go#L35-L42](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/x/distribution/types/tx.pb.go#L35-L42) The signature check is performed for `Sender` and not the `DelegatorAddress`. The missing check allows a user to set a withdrawal address for another user. ```go=34 func (msg MsgSetWithdrawAddress) GetSigners() []sdk.AccAddress { senderAddr, err := sdk.AccAddressFromBech32(msg.Sender) if err != nil { panic(err) } return []sdk.AccAddress{senderAddr} } ``` [distribution/types/msgs.go#L34-L40](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/x/distribution/types/msgs.go#L34-L40) #### Impact A malicious user can leverage this, set an alternative withdrawal address for all delegators in the system, and withdraw their staking rewards. The SGNv2 is configured so that `MsgSetWithdrawAddress` is deactivated at the moment and reverts when called. #### Remediation The `MsgSetWithdrawAddress` endpoint should be removed from the codebase to ensure that the endpoint is never active and accessible. ### 7. Same events can be submitted multiple times | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **Low** |Acknowledged | | _Response from the team: No security risk. Will improve in later upgrades._| #### Description There are no checks on the `Success` attribute if the`PegbrEventWithdrawn` and `PegbrEventMint` event with a specific id has been applied already. ```go=144 case types.PegbrEventMint: // Mint event different bridge versions are the same ptbContract, _ := eth.NewPeggedTokenBridgeFilterer(eth.ZeroAddr, nil) ev, err := ptbContract.ParseMint(*elog) if err != nil { return false, err } mintInfo, found := k.GetMintInfo(ctx, ev.MintId) if !found { log.Errorln("x/pegbr mint info not found", ev.PrettyLog(onchev.Chainid)) return false, nil } mintInfo.Success = true k.SetMintInfo(ctx, ev.MintId, mintInfo) log.Infoln("x/pegbr applied:", ev.PrettyLog(onchev.Chainid)) return true, nil case types.PegbrEventWithdrawn: // Withdrawn event different vault versions are the same otvContract, _ := eth.NewOriginalTokenVaultFilterer(eth.ZeroAddr, nil) ev, err := otvContract.ParseWithdrawn(*elog) if err != nil { return false, err } wdInfo, found := k.GetWithdrawInfo(ctx, ev.WithdrawId) if !found { log.Errorln("x/pegbr withdraw info not found", ev.PrettyLog(onchev.Chainid)) return false, nil } wdInfo.Success = true k.SetWithdrawInfo(ctx, ev.WithdrawId, wdInfo) log.Infoln("x/pegbr applied:", ev.PrettyLog(onchev.Chainid)) return true, nil } ``` [keeper/apply.go#L144-L177](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/x/pegbridge/keeper/apply.go#L144-L177) There are no checks if the `CbrEventRelay` and `CbrEventWithdraw` events have been applied already. ```go=125 case types.CbrEventRelay: // relay happened on dest chain ev, err := cbrContract.ParseRelay(*elog) if err != nil { return false, err } SetEvSendStatus(kv, ev.SrcTransferId, types.XferStatus_SUCCESS) log.Infoln("x/cbr applied:", ev.PrettyLog(onchev.Chainid)) case types.CbrEventWithdraw: ev, err := cbrContract.ParseWithdrawDone(*elog) if err != nil { return false, err } log.Infoln("x/cbr apply withdrawDone", ev.String()) wdDetail := GetWithdrawDetail(kv, ev.Receiver, ev.Seqnum) if wdDetail == nil { // what to do if not found? return true, nil } wdDetail.Completed = true SaveWithdrawDetail(kv, ev.Receiver, ev.Seqnum, wdDetail) if wdDetail.XferId != nil { // this is a refund so we set xfer status to refund_done SetEvSendStatus(kv, eth.Bytes2Hash(wdDetail.XferId), types.XferStatus_REFUND_DONE) } log.Infoln("x/cbr applied:", ev.PrettyLog(onchev.Chainid)) ``` [keeper/apply.go#L125-L150](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/x/cbridge/keeper/apply.go#L125-L150) #### Impact There could be potential side effects when the same event is applied multiple times. #### Remediation Skip applying an event that has already been applied before. ### 8. `Pegbridge` verification functions do not consistently check logs | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **Low** |Fixed | [pegbr_verifier.go](https://github.com/celer-network/sgnv2/blob/b4d11dc28b40f4899416cf3f6762ad872ef1b597/relayer/pegbr_verifier.go)| `verifyPegbrWithdrawn`, `verifyPegbrBurn` and `verifyPegbrMint` use the `verifyEventLog` function which checks the block delay for a specific chain (see also https://github.com/celer-network/sgn-v2-networks/blob/main/sgn-3/cbridge.toml)| #### Description The `verifyPegbrDeposit` function, which checks pending updates for Pegbridge deposits, does not verify the `Deposited` log. Instead, it calculates the `DepositIdV2` from all values submitted in the pending update and checks if it exists in the `OriginalTokenVaultV2` contract. ```go=101 } else if eLog.Address == c.pegContracts.vault2.GetAddr() && eLog.Address != eth.ZeroAddr { ev, err := c.pegContracts.vault2.ParseDeposited(*eLog) if err != nil { log.Errorf("%s. parse eLog error %s", logmsg, err) return true, false } logmsg = fmt.Sprintf("%s. %s", logmsg, ev.String()) depositId := pegtypes.CalcDepositIdV2( ev.Depositor, ev.Token, ev.Amount, ev.MintChainId, ev.MintAccount, ev.Nonce, c.chainid, eLog.Address) if depositId != ev.DepositId { log.Errorf("%s. mismatch depositId ev has %x, calc: %x", logmsg, ev.DepositId, depositId) return true, false } return c.verifyOriginalTokenRecord(ev.DepositId, eLog.BlockNumber, cliCtx, logmsg, true) } ``` [relayer/pegbr_verifier.go#L101-L115](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/pegbr_verifier.go#L101-L115) While it was not possible to circumvent this check during the review, it is still recommended to fetch the log from on-chain and then compare all attributes against the event in the pending update like it is done in the `verifyEventLog` function. Other verification functions that do not use this pattern consistently are listed below: - [verifyPegbrWithdrawn](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/pegbr_verifier.go#L121-L153) - [verifyPegbrBurn](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/pegbr_verifier.go#L180-L193) - [verifyPegbrMint](https://github.com/celer-network/sgn-v2/blob/ac26184f4cb868b394d7b02cc6384eae6b42fe80/relayer/pegbr_verifier.go#L199-L230) #### Impact If a malicious user is able to circumvent the log verification, then it is possible to submit pending updates to the SGNv2 that did not actually take place on a connected chain. #### Remediation Consistently verify that an action took place on-chain by fetching and checking the logs with `verifyEventLog` and then comparing all attributes from the pending update log. ### 9. Missing signature length check in `RecoverSigner` | Severity | Status | Link | Remediation Comment | | ------------- | ------------- | ------------- | ------------- | | **Low** |Fixed | [crypto.go#L95-L97](https://github.com/celer-network/goutils/blob/master/eth/crypto.go#L95-L97) | Only allows signatures with 65 bytes length | #### Description The `RecoverSigner` function is used throughout the application to retrieve the signer address from signed data. There is no check on the length of the signature, and an empty signature can be submitted. ```go=93 func RecoverSigner(data []byte, sig []byte) (common.Address, error) { // clone one to use, to make sure the original sig won't be changed by me tmpSig := make([]byte, len(sig)) copy(tmpSig, sig) if len(tmpSig) == 65 { // we could return zeroAddr if len not 65 if tmpSig[64] == 27 || tmpSig[64] == 28 { // SigToPub only expect v to be 0 or 1, // see https://github.com/ethereum/go-ethereum/blob/v1.8.23/internal/ethapi/api.go#L468. // we've been ok as our own code only has v 0 or 1, but using external signer may cause issue // we also fix v in celersdk.PublishSignedResult to be extra safe tmpSig[64] -= 27 } } pubKey, err := crypto.SigToPub(GeneratePrefixedHash(data), tmpSig) if err != nil { return common.Address{}, err } recoveredAddr := crypto.PubkeyToAddress(*pubKey) return recoveredAddr, nil } ``` [eth/crypto.go#L93-L113](https://github.com/celer-network/goutils/blob/f461da5e1fdf14021e020ed52f0a544a22e878ed/eth/crypto.go#L93-L113) #### Impact A malicious user could potentially circumvent the signature recovery. #### Remediation Only process signatures with valid length. ## Detailed recommendations ### Restrict messages The following subchapters contain a list of all endpoints and their permission settings. A red font test is used for recommendations on where action should be taken to set stricter permissions. #### sync module | Message | Status | Permission | | -------- | -------- | -------- | | [MsgProposeUpdates](https://github.com/celer-network/sgn-v2/blob/main/x/sync/spec/02_messages.md#msgproposeupdates) | Reviewed | Currently all users Recommended to limit to bonded validators | | [MsgVoteUpdates](https://github.com/celer-network/sgn-v2/blob/main/x/sync/spec/02_messages.md#msgvoteupdates) | Reviewed | Currently all users Recommended to limit to bonded validators | #### cbridge module | Message | Status | Permission | | -------- | -------- | -------- | | [MsgInitWithdraw](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/cbridge/spec/03_messages.md) | Reviewed | All users | | [MsgSendMySig](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/cbridge/spec/03_messages.md) | Reviewed |Bonded validators| | [MsgSignAgain](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/cbridge/spec/03_messages.md) | Reviewed| All users| | [MsgUpdateLatestSigners](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/cbridge/spec/03_messages.md) | Reviewed | Bonded validators | | [MsgSyncFarming](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/cbridge/spec/03_messages.md) | Reviewed | All users| #### staking module | Message | Status | Permission | | -------- | -------- | -------- | | [MsgSetTransactors](https://github.com/celer-network/sgn-v2/blob/main/x/staking/spec/02_messages.md#msgsettransactors) | Reviewed | Bonded validators | | [MsgEditDescription](https://github.com/celer-network/sgn-v2/blob/main/x/staking/spec/02_messages.md#msgeditdescription) | Reviewed | Bonded and unbonded validators | #### distribution module | Message | Status | Permission | | -------- | -------- | -------- | | [MsgSetWithdrawAddress](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/distribution/spec/04_messages.md#msgsetwithdrawaddress) | Will be removed | N/A | | [MsgWithdrawDelegatorReward](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/distribution/spec/04_messages.md#msgwithdrawdelegatorreward) | Reviewed | All users | | [MsgWithdrawValidatorCommission](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/distribution/spec/04_messages.md#msgwithdrawvalidatorcommission) | Reviewed | All users | | [MsgFundCommunityPool](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/distribution/spec/04_messages.md#msgfundcommunitypool) | Reviewed | All users | | [MsgClaimAllStakingReward](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/distribution/spec/04_messages.md#msgclaimallstakingreward) | Reviewed | All users | | [MsgSignStakingReward](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/distribution/spec/04_messages.md#msgsignstakingreward) | Reviewed |Bonded validators | #### pegbridge | Message | Status | Permission | | -------- | -------- | -------- | | [MsgSignMint](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/pegbridge/spec/03_messages.md#msgsignmint) | Reviewed | Bonded validators | | [MsgSignWithdraw](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/pegbridge/spec/03_messages.md#msgsignwithdraw) |Reviewed | Bonded validators | | [MsgTriggerSignMint](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/pegbridge/spec/03_messages.md#msgtriggersignmint) | Reviewed | All users | | [MsgTriggerSignWithdraw](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/pegbridge/spec/03_messages.md#msgtriggersignwithdraw) | Reviewed | All users | | [MsgClaimFee](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/pegbridge/spec/03_messages.md#msgclaimfee) | Reviewed | All users | | [MsgClaimRefund](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/pegbridge/spec/03_messages.md#msgclaimrefund) | Reviewed | All users | #### farming | Message | Status | Permission | | -------- | -------- | -------- | | [MsgClaimRewards](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/farming/spec/03_messages.md#msgclaimallrewards) | Not activated | N/A | | [MsgClaimAllRewards](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/farming/spec/03_messages.md#msgclaimallrewards) | Reviewed | All users | | [MsgSignRewards](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/farming/spec/03_messages.md#msgsignrewards) | Reviewed | Bonded validators | #### gov | Message | Status | Permission | | -------- | -------- | -------- | | [MsgSubmitProposal](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/gov/spec/03_messages.md#proposal-submission) | Reviewed | All users | | [MsgDeposit](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/gov/spec/03_messages.md#deposit) | Reviewed | Currently bonded and unbonded validators Recommended to limit to bonded validators| | [MsgVote](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/gov/spec/03_messages.md#vote) | Reviewed | Currently all users Recommended to limit to bonded validators | #### message | Message | Status | Permission | | -------- | -------- | -------- | | MsgSignMessage | Reviewed | Bonded validators | | MsgTriggerSignMessage | Not Implemented | N/A | | MsgClaimAllFees | Reviewed | All users | | MsgSignFees | Reviewed | Bonded validators | #### slashing | Message | Status | Permission | | -------- | -------- | -------- | | [MsgSignSlash](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/slashing/spec/03_messages.md#signslash) | Reviewed | Bonded validators | ### Update documentation Find below the list of documentation locations that have been provided for the security review and comments on their quality. | Site/Module | quality of documentation | | -------- | -------- | | Main documentation sites at https://www.celer.network/docs/celercore/ and https://cbridge-docs.celer.network/ | **~~The sites lack a clear high-level description of what the SGNv2 is and how it works. Architecture and Design documentation seems to describe a previous version, and there is a clear gap to the current implementation.~~** **Additional documentation has been provided during the security review that describes [concepts and workflows](https://github.com/celer-network/sgn-v2/blob/main/docs/workflows.md) and the [relayer component](https://github.com/celer-network/sgn-v2/blob/main/docs/relayer.md) of the SGNv2. The main documentation sites should still be updated.** | | [staking](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/staking/spec/README.md) |Adequately describes the functionality of the module | | [sync](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/sync/spec/README.md) | Adequately describes the functionality of the module | | [cbridge](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/cbridge/spec/README.md) | **Message descriptions are missing and the formatting of the spec is broken.** | | [pegbridge](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/pegbridge/spec/README.md) | Adequately describes the functionality of the module| | [distribution](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/distribution/spec/README.md) | Adequately describes the functionality of the module| | [farming](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/farming/spec/README.md) | Adequately describes the functionality of the module| | [gov](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/gov/spec/README.md) | Adequately describes the functionality of the module | | [message](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/message/spec/README.md) | **Message descriptions seem to be outdated.** | | [slashing](https://github.com/celer-network/sgn-v2/blob/f6efbd6ee1aa1c9c1e488d7cff86bdcd2571e705/x/slashing/spec/README.md) | Adequately describes the functionality of the module| ## Appendix ### Test systems The following test systems have been available during the security review: - Public testnet was available at https://dev-cbridge-v2.netlify.app/ and https://test-sgn.celer.network/. - A local test network was set up with the following instructions [here](https://github.com/celer-network/sgn-v2/blob/main/test/e2e/manual/docs/fullstack.md). ### Issue severity Issues are classified based on CVSSv3 severity categories. Risk scoring is not rigorously applied as CVSSv3 calculation is more optimized for web2 applications, and a score would be less meaningful. Instead, issues are labeled based on the following category guidelines. #### Critical Critical issues are directly exploitable bugs or security vulnerabilities. These issues are likely to cause significant economic loss for the system. #### High High issues are security issues that have some preconditions or that require specific scenarios to be in place to make them exploitable. The economic loss that could be caused by them might also be limited. #### Medium Medium issues do not have a direct path to exploitation. A malicious user might combine them with other issues to increase the impact of an attack. #### Low Low issues typically are around establishing security best practices or defense in depth measures.

Syntax	Example	Reference
# Header	Header	基本排版
- Unordered List	Unordered List
1. Ordered List	Ordered List
- [ ] Todo List	Todo List
> Blockquote	Blockquote
Bold font	Bold font
Italics font	Italics font
~~Strikethrough~~	~~Strikethrough~~
19^th^	19^th
H~2~O	H₂O
++Inserted text++	Inserted text
==Marked text==	Marked text
[link text](https:// "title")	Link
![image alt](https:// "title")	Image
`Code`	`Code`	在筆記中貼入程式碼
```javascript var i = 0; ```	`var i = 0;`
:smile:		Emoji list
{%youtube youtube_id %}	Externals
$L^aT_eX$	L^aT_eX
:::info This is a alert area. :::	This is a alert area.