code-423n4 / 2022-10-thegraph-findings

0 stars 0 forks source link

GRT may be locked in the destination contract forever if the user or external developers bridge it to a contract that requires onTokenTransfer without sending data. #168

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/gateway/L1GraphTokenGateway.sol#L213-L216

Vulnerability details

Impact

GRT may be locked in the destination contract forever if the user or external developers bridge it to a contract that requires onTokenTransfer without sending data.

Since users or external developers may not understand GRT bridging correctly. They may bridge to a contract in the destination chain that requires onTokenTransfer in the hope that it will be executed.

Proof of Concept

                require(
                    extraData.length == 0 || callhookWhitelist[msg.sender] == true,
                    "CALL_HOOK_DATA_NOT_ALLOWED"
                );

Users or external developers can't send callhook but can bridge GRT to a contract on the destination chain that requires onTokenTransfer. These contract may not contains any logic to handle fund sending without calling onTokenTransfer.

Recommended Mitigation Steps

Add another whitelist to disallow users or external developers to send fund to these contracts

                require(
                    (extraData.length == 0 && !destinationBlacklisted[_to]) || callhookWhitelist[msg.sender] == true,
                    "CALL_HOOK_DATA_NOT_ALLOWED"
                );
0xean commented 1 year ago

The expectation of maintaining a black list is not reasonable here. This really boils down to a developer (an advanced user) attempting to send tokens in a way they don't understand. Seems very similar to a user sending tokens to contract incorrectly on the same chain and them becoming trapped. Downgrading to QA