Freeze Bridge via Non-UTF8 Token Name/Symbol/Denom

[code423n4]

Handle

nascent

Vulnerability details

Manual insertion of non-utf8 characters in a token name will break parsing of logs and will always result in the oracle getting in a loop of failing and early returning an error. The fix is non-trivial and likely requires significant redesign.

Proof of Concept

Note the c0 in the last argument of the call data (invalid UTF8).

It can be triggered with:

data memory bytes = hex"f7955637000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000012000000000000000000000000000000000000000000000000000000000000000461746f6d0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000046e616d6500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000673796d626fc00000000000000000000000000000000000000000000000000000";
gravity.call(data);

The log output is as follows:

ERC20DeployedEvent("atom", "name", ❮utf8 decode failed❯: 0x73796d626fc0, 18, 2)

Which hits this code path:

            let symbol = String::from_utf8(input.data[index_start..index_end].to_vec());
            trace!("Symbol {:?}", symbol);
            if symbol.is_err() {
                return Err(GravityError::InvalidEventLogError(format!(
                    "{:?} is not valid utf8, probably incorrect parsing",
                    symbol
                )));
            }

And would cause an early return here:

let erc20_deploys = Erc20DeployedEvent::from_logs(&deploys)?;

Never updating last checked block and therefore, this will freeze the bridge by disallowing any attestations to take place. This is an extremely low cost way to bring down the network.

Recommendation

This is a hard one. Resyncing is permanently borked because on the Go side, there is seemingly no way to ever process the event nonce because protobufs do not handle non-utf8 strings. The validator would report they need event nonce N from the orchestrator, but they can never parse the event N. Seemingly, validators & orchestrators would have to know to ignore that specific event nonce. But it is a permissionless function, so it can be used to effectively permanently stop attestations & the bridge until a new Gravity.sol is deployed.

One potential fix is to check in the solidity contract if the name contains valid utf8 strings for denom, symbol and name. This likely will be expensive though. Alternatively, you could require that validators sign ERC20 creation requests and perform checks before the transaction is sent.

[jkilpatr]

This is a valid and well considered bug.

I do disagree about the difficulty of the fix though, if we fail to parse the token name as utf8 we can just encode the bytes themselves in hex and pass that along. The result will be perfectly valid if a little unergonomic.

[albertchon]

Clever, great catch

[loudoguno]

reopening as per judges assessment as "primary issue" on findings sheet

[taitruong]

This is a valid and well considered bug.

I do disagree about the difficulty of the fix though, if we fail to parse the token name as utf8 we can just encode the bytes themselves in hex and pass that along. The result will be perfectly valid if a little unergonomic.

@jkilpatr, has this been fixed? Can't find an issue on Gravity repo. Just curious how this bug may gets solved.

[jkilpatr]

Integration test that tests the fix every commit: https://github.com/Gravity-Bridge/Gravity-Bridge/blob/main/orchestrator/test_runner/src/invalid_events.rs Integration test output: https://github.com/Gravity-Bridge/Gravity-Bridge/actions/runs/3062208028/jobs/4943365412

Please let me know if you can think of any test cases not in that test.