PoolTogether contest - ladboy233's results

Lines of code

https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol#L159 https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-optimism/EthereumToOptimismRelayer.sol#L64 https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-polygon/EthereumToPolygonRelayer.sol#L60

Vulnerability details

Impact

Cross-chain request does not have expiration time

Proof of Concept

In the current implementation, a caller in source chain can perform a relayer call to starts a cross-chain request.

But it is not clearly when the executor will be executing the cross-chain.

Some blockchain transaction can be very time senstiive (trading, etc...)

For example, a user wants to perform a cross-chain from ethereum to polygon,

In the polygon destination chain, the user wants to borrow 1000 USDC using 2 ETH in a lending market.

at the time when user performs a request, 1 ETH = 1300 USD.

but the executor is very busy and does not execute the cross-chain request until two days later, at this time, the ETH price drops to 500 USD per ETH or below 500 USD. the borrow request is executed but the user's position is liquidated.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project adding a expiration time when creating the cross-chain request on the relayer side, and on execution side, check if the cross-chain is expired, if expired, do not perform the request.

[L-01] nonce should be marked as public instead of internal.

Line Of Code:

https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-optimism/EthereumToOptimismRelayer.sol#L29

https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-polygon/EthereumToPolygonRelayer.sol#L23

https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol#L40

Vulnerability Detail and recommended fix

in the current implementation, the nonce is set to internal, and the user is only able to know the nonce by querying the blockchain transaction and read the emitted event log or use the return value from relayCall, which is very inconvenient to keep track of the nonce.

/// @notice Nonce to uniquely idenfity each batch of calls.
uint256 internal nonce;

We recommend the codebase change the access control for nonce from internal to public.

[L-02] unsafe downcasting of the gas limit from uint256 to uint32 in OptimismRelayer.sol

Line Of Code:

https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-optimism/EthereumToOptimismRelayer.sol#L72

Vulnerability Detail and recommended fix

The gas limit is unsafely downcasted from uint256 to uint32 in OptimismRelayer.sol

crossDomainMessenger.sendMessage(
  address(executor),
  abi.encodeWithSignature(
	"executeCalls(uint256,address,(address,bytes)[])",
	_nonce,
	msg.sender,
	_calls
  ),
  uint32(_gasLimit)
);

the gas limit can be truncated if the gas limit that is larger than uint32 is set.

We recommend the project use openzepplin unsafe case to make sure the gas limit is not trancunated sliently.