Gala Games - TON Bridge

CertiK22 Nov 2024Code version: not recorded (see the report)

CertiK reviewed Gala (Gala Games - TON Bridge) on 22 Nov 2024: 15 findings, 1 critical; 14 marked fixed, 1 acknowledged.

1 critical finding, fixed

Open the report Open Gala’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings15

Statuses are the security firm’s own, from its report.

Severity	Acknowledged	Fixed
Critical	0	1
High	1	1
Medium	0	2
Low	0	10

CriticalSignature can be fakedFixed

```c=20 (int, int, int, int) ecrecover (int hash, int v, int r, int s) asm "ECRECOVER"; ... (int h, int x1, int x2, int success) = ecrecover(packet_hash, v, r, s); slice rec_pubkey = begin_cell().store_uint(h, 8).store_uint(x1, 256).store_uint(x2, 256).end_cell().begin_parse(); throw_unless(error::signature, rec_pubkey.equal_slice_bits(pubkey)); ``` `ECRECOVER` opcode [returns](https://docs.ton.org/learn/tvm-instructions/tvm-upgrade-2023-07#secp256k1) either 0 or `h x1 x2 -1`. In case 0 is returned, only **one value is pushed on stack**. Extracting 4 values will populate `(int h, int x1, int x2, int success)` with the values that were already on top of stack. These values are `r, s, v, success` completely controlled by the caller. The `success` output is **not checked** by the function. As a result, the caller can craft the signature (520 bits) that will satisfy the condition `rec_pubkey.equal_slice_bits(pubkey)` and pass the check. All funds can be stolen.

HighPacket signatures checking is flawedFixed

In `bridge_gateway` `op::route_unlock` handler the signature check is handled incorrectly: 1. It cycles `until(signatures.slice_refs_empty?())` but never updates the `signatures` with the next ref. Same signature will be used over and over again. 2. It doesn't validate the number of signatures provided. No threshold is set, one signature is always enough. 3. It doesn't ensure that the signatures are from different verifiers. Same signature can be repeated several times.

MediumLack of replay protectionFixed

The signed messages don't follow the [EIP-712 recommendations](https://eips.ethereum.org/EIPS/eip-712#specification). The domain separator doesn't include: 1. `name` - the user readable name of signing domain, i.e. the name of the DApp or the protocol. 2. `verifyingContract` - the address of the contract that will verify the signature. 3. `encodeType` - like "op::route_unlock(args)"h As a result, the message signed by verifiers for gateway in one chain (i.e. testnet) can be replayed on another one, or on another gateway deployment. Or the message signed with another purpose can be submitted to `op::route_unlock` handler.

MediumWrong gas management in `op::route_unlock`Fixed

`bridge_gateway` `op::route_unlock` handler doesn't check that the gas provided is enough to finish the workflow. Lack of gas can lead to incomplete transaction execution.

Low`end_parse()` Is MissingFixed

The data is packed in slice and parsed by the contract. To ensure the slice has the expected data structure and no data left after parsing, `end_parse()` can be applied to the slice. If slice is empty, it throws an exception.

Low`jetton_bridge` doesn't allow fees collectingFixed

`jetton_bridge` doesn't allow the owner to collect fees. `ton_coin_bridge` doesn't allow the owner to change the flat fee.

Low`load_grams()` is deprecatedFixed

`load_grams()` is deprecated. `load_coins()` should be used instead.

Low`wallet_address` is used as `response_destinationFixed

`response_destination` in `op::transfer` message is used to accept gas excesses. Using `wallet_address` is unreasonable since it is not expected to collect extra gas.

LowCorrect workchain not enforcedFixed

```c=2 (_, int wallet_hash) = parse_std_addr(wallet); ``` It is not ensured the address workchain is basechain. Unexpected messages from the masterchain can break the contract invariants.

LowInconsistent payload serializingFixed

`decode_payload()` reads `extra = payload~load_maybe_ref()`. `load_maybe_ref()` loads 1 bit and if it is true, loads first ref and returns it with slice remainder. However, `encode_payload()` doesn't store that 1 bit. Trying to `decode_payload()` that was encoded by `encode_payload()` will result in cell underflow.

Show 4 more

LowLack of sanity checksFixed

```c=89 amount -= flat_fee; ``` It is not checked that `amount` is bigger than `flat_fee`. As a result, the transaction will fail with integer underflow during `store_coins()`.

LowOutdated commentsFixed

Some comments are outdated. - `lock_ton#1bc9f191` is in fact `0x6372e897` - `unlock#1d1c0c53 amount:Grams instance:uint(256) recipient:^RecipientBody wallet_hash:uint(256) = InternalMsgBody;` `unlock` tag is in fact 5. The message payload is stored in the separate cell. `Grams` should be `Coins`. - etc.

LowSusceptible to Signature MalleabilityFixed

The signature malleability is possible within the Elliptic Curve cryptographic system. An Elliptic Curve is symmetric on the X-axis, meaning two points can exist with the same `X` value. In the `r`, `s` and `v` representation this permits us to carefully adjust `s` to produce a second valid signature for the same `r`, thus breaking the assumption that a signature cannot be replayed in what is known as a replay-attack. Appendix F in the [Ethereum Yellow paper](https://ethereum.github.io/yellowpaper/paper.pdf) defines the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}. ```c=144 if(v >= 31) { v = v - 31; } ``` The current code allows bigger `v` and doesn't check `s`.

LowUnsafe handle of `op::transfer_notification`Fixed

`jetton_bridge::op::transfer_notification` handler works this way: 1. unpacks `op::transfer_notification` message 2. ensures `msg_value >= flat_fee` 3. unpacks the payload of `op::transfer_notification` message 4. sends `op::log_request` to the gateway However, if the `msg_value` is not enough or payload is in a wrong format, the tokens will not be returned to the owner.