code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Possible lost ETH in L1GraphTokenGateway.outboundTransfer().

Proof of Concept

L1GraphTokenGateway.outboundTransfer() is a payable function and it checks with msg.value like below.

uint256 expectedEth = maxSubmissionCost.add(_maxGas.mul(_gasPriceBid));
require(msg.value >= expectedEth, "WRONG_ETH_VALUE");

As we can see here, maxSubmissionCost is extracted inside the function and it would be difficult to know the correct expectedEth before the function call.

So it's possible that a user might send more msg.value than he should and the exedent ETH will lost.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend adding the ETH refund logic here.

require(msg.value >= expectedEth, "WRONG_ETH_VALUE");

if(msg.value > expectedEth) {
    (bool success, ) =
        payable(msg.sender).call{value: msg.value - expectedEth}("");

    require(success, "ETH refund failed");
}

0xean commented 1 year ago

I believe this is invalid, and the exact amount is unknown at the time the value is transferred to the L2 - will leave open for sponsor to confirm understanding.