Missing contract existence checks for low-level calls

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/48e7c7cf641847e07ba07e01176cb17ba8ad6432/contracts/statechannels/GRTWithdrawHelper.sol#L56-L78

Vulnerability details

Impact

When doing low-level calls, it's necessary to not only check that the address is non-null, but that it has actual code. When a low-level call is made on a valid address that has no deployed code, the low level call always returns true. This may be the case of a create2() address hasn't yet had its contract deployed, or the contract has been destructed.

In this case, the code uses the low level call as a sort of check for approval, assuming that failure to approve will block the transfer of the tokens that comes after it. If tokenAddress does not have any code deployed, the call to APPROVE_SELECTOR will pass, and the COLLECT_SELECTOR transfer that comes afterwards will be done

Proof of Concept

// File: contracts/statechannels/GRTWithdrawHelper.sol : GRTWithdrawHelper.execute()   #1

56        function execute(WithdrawData calldata _wd, uint256 _actualAmount) external override {
57            require(_wd.assetId == tokenAddress, "GRTWithdrawHelper: !token");
58    
59            // Decode and validate collect data
60            CollectData memory collectData = abi.decode(_wd.callData, (CollectData));
61            require(collectData.staking != address(0), "GRTWithdrawHelper: !staking");
62            require(collectData.allocationID != address(0), "GRTWithdrawHelper: !allocationID");
63            require(collectData.returnAddress != address(0), "GRTWithdrawHelper: !returnAddress");
64    
65            // Approve the staking contract to pull the transfer amount
66 @>         (bool success1, ) = tokenAddress.call(
67 @>             abi.encodeWithSelector(APPROVE_SELECTOR, collectData.staking, _actualAmount)
68 @>         );
69    
70            // If the call fails return the funds to the return address and bail
71            if (!success1) {
72                _sendTokens(collectData.returnAddress, _actualAmount);
73                return;
74            }
75    
76            // Call the Staking contract to collect funds from this contract
77 @>         (bool success2, ) = collectData.staking.call(
78:@>             abi.encodeWithSelector(COLLECT_SELECTOR, _actualAmount, collectData.allocationID)

https://github.com/code-423n4/2022-10-thegraph/blob/48e7c7cf641847e07ba07e01176cb17ba8ad6432/contracts/statechannels/GRTWithdrawHelper.sol#L56-L78

Tools Used

Code inspection

Recommended Mitigation Steps

require() that the <address>.code.length is non-zero before each low-level call

[0xean]

Given that the token in question is the graph token, which is immutably set in the constructor this seems very unlikely to be an issue here. I think at best this could be QA.

[0xean]

closing as dupe of #118 - wardens QA

[pcarranzav]

Also out of scope (GRTWithdrawHelper.sol)