The postRelayedCall function of the TokenPaymaster contract is public and allows anyone to call it. In this function the uniswap router is called and leftover funds are sent back to the user. Thus, this function modifies the state. Without an access control check that only allows the RelayHub to call it, an attacker can craft call data that will drain the ERC20 balance of the contract. Note that normally the contract is not supposed to have an ERC20 balance, however, this can change in situations as described in Anyone can steal GSN fees.
The preRelayedCall and postRelayedCall functions both have following comment in the official GSN IPaymaster class:
The preRelayedCall function does not modify state but it could still make sense to add an access control check.
2
The contracts use an outdated GSN interface in the definition of IPaymaster. In particular the following differences exist:
The function getGasLimits should now be called getGasAndDataLimits() and should return four instead of three values.
A trustedForwarder function should be implemented.
Furthermore, the RelayData struct has changed. As less context information is available the security guarantees are weaker.
The updates in this PR consist of:
1) Add relayHubOnly modifier for postRelayedCall
2) Update GSN Interface:
1) Add GasAndDataLimits struct and trustedForwarder function in IPaymaster interface
2) Update RelayData struct
3) Add claimTokens function in TokenPaymaster
1
The
postRelayedCallfunction of theTokenPaymastercontract is public and allows anyone to call it. In this function the uniswap router is called and leftover funds are sent back to the user. Thus, this function modifies the state. Without an access control check that only allows theRelayHubto call it, an attacker can craft call data that will drain the ERC20 balance of the contract. Note that normally the contract is not supposed to have an ERC20 balance, however, this can change in situations as described in Anyone can steal GSN fees.The
preRelayedCallandpostRelayedCallfunctions both have following comment in the official GSNIPaymasterclass:https://github.com/poanetwork/tokenbridge-contracts/blob/b3511bf0987bbfef661e28dd1a6fbe1735f90ac0/contracts/gsn/interfaces/IPaymaster.sol#L57
The
preRelayedCallfunction does not modify state but it could still make sense to add an access control check.2
The contracts use an outdated GSN interface in the definition of
IPaymaster. In particular the following differences exist:getGasLimitsshould now be calledgetGasAndDataLimits()and should return four instead of three values.trustedForwarderfunction should be implemented.Furthermore, the
RelayDatastruct has changed. As less context information is available the security guarantees are weaker.The updates in this PR consist of:
1) Add
relayHubOnlymodifier forpostRelayedCall2) Update GSN Interface: 1) AddGasAndDataLimitsstruct andtrustedForwarderfunction inIPaymasterinterface 2) UpdateRelayDatastruct 3) AddclaimTokensfunction inTokenPaymaster