I've competed in this contest between 07/10/22-12/10/22 and achieved first place. The contest covered the L2 bridge component of the Graph's infrastructure. Repo is here.
MED: 2
MED: After proposed 0.8.0 upgrade kicks in, L2 finalizeInboundTransfer might not work. 🚩
Description
L2GraphTokenGateway uses the onlyL1Counterpart modifier to make sure finalizeInboundTransfer is only called from L1GraphTokenGateway. Its implementation is:
modifier onlyL1Counterpart() {
require(
msg.sender == AddressAliasHelper.applyL1ToL2Alias(l1Counterpart),
"ONLY_COUNTERPART_GATEWAY"
);
_;
}It uses applyL1ToL2Alias defined as:
uint160 constant offset = uint160(0x1111000000000000000000000000000000001111);
/// @notice Utility function that converts the address in the L1 that submitted a tx to
/// the inbox to the msg.sender viewed in the L2
/// @param l1Address the address in the L1 that triggered the tx to L2
/// @return l2Address L2 address as viewed in msg.sender
function applyL1ToL2Alias(address l1Address) internal pure returns (address l2Address) {
l2Address = address(uint160(l1Address) + offset);
}This behavior matches with how Arbitrum augments the sender's address to L2. The issue is that I've spoken with the team and they are planning an upgrade from Solidity 0.7.6 to 0.8.0. Their proposed changes will break this function, because under 0.8.0, this line has a ~ 1/15 chance to overflow:
l2Address = address(uint160(l1Address) + offset);Interestingly, the sum intentionally wraps around using the uint160 type to return a correct address, but this wrapping will overflow in 0.8.0
Impact
There is a ~6.5% chance that finalizeInboundTransfer will not work.
Proof of Concept
l1Address is L1GraphTokenGateway, suppose its address is 0xF000000000000000000000000000000000000000.
Then 0xF000000000000000000000000000000000000000 + 0x1111000000000000000000000000000000001111 > UINT160_MAX , meaning overflow.
Tools Used
Manual audit
Recommended Mitigation Steps
Wrap the calculation in an unchecked block, which will make it behave correctly.
MED: If L1GraphTokenGateway's outboundTransfer is called by a contract, the entire msg.value is blackholed, whether the ticket got redeemed or not.🚩
Description
The outboundTransfer function in L1GraphTokenGateway is used to transfer user's Graph tokens to L2. To do that it eventually calls the standard Arbitrum Inbox's createRetryableTicket. The issue is that it passes caller's address in the submissionRefundAddress and valueRefundAddress. This behaves fine if caller is an EOA, but if it's called by a contract it will lead to loss of the submissionRefund (ETH passed to outboundTransfer() minus the total submission fee), or in the event of failed L2 ticket creation, the whole submission fee. The reason it's fine for EOA is because of the fact that ETH and Arbitrum addresses are congruent. However, the calling contract probably does not exist on L2 and even in the rare case it does, it might not have a function to move out the refund.
The docs don't suggest contracts should not use the TokenGateway, and it is fair to assume it will be used in this way. Multisigs are inherently contracts, which is one of the valid use cases. Since likelihood is high and impact is medium (loss of submission fee), I believe it to be a HIGH severity find.
Impact
If L1GraphTokenGateway's outboundTransfer is called by a contract, the entire msg.value is blackholed, whether the ticket got redeemed or not.
Proof Of Concept
Alice has a multisig wallet. She sends 100 Graph tokens to L1GraphTokenGateway, and passes X ETH for submission. She receives an L1 ticket, but since the max gas was too low, the creation failed on L2 and the funds got sent to the multisig address at L2. Therefore, Alice loses X ETH.
Tools Used
Recommended mitigation steps
A possible fix is to add an isContract flag. If sender is a contract, require the flag to be true.
Another option is to add a refundAddr address parameter to the API.