Skip to content

Update GSN interface #628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 20, 2021
Merged

Update GSN interface #628

merged 2 commits into from
Aug 20, 2021

Conversation

lok52
Copy link

@lok52 lok52 commented Aug 19, 2021

1

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:

https://github.com/poanetwork/tokenbridge-contracts/blob/b3511bf0987bbfef661e28dd1a6fbe1735f90ac0/contracts/gsn/interfaces/IPaymaster.sol#L57

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

@lok52 lok52 added audit Items related to the security audits team-august-2021 Related to items found by ChainSecurity in the audit held in August of 2021 labels Aug 19, 2021
@lok52 lok52 self-assigned this Aug 19, 2021
@k1rill-fedoseev k1rill-fedoseev self-requested a review August 19, 2021 10:51
@akolotov akolotov merged commit 93b1afb into develop Aug 20, 2021
@akolotov akolotov deleted the audit/august2021/5.3/5.4 branch August 20, 2021 09:48
akolotov added a commit that referenced this pull request Sep 6, 2021
This update for the `master` branch contains the changes made to address findings discovered during a security audit:
 * [Fix] Stricter preconditions for payInterest (#623)
 * [Fix] Fix offset in comments (#624)
 * [Fix] Use fixed lower call gas limit (#627)
 * [Fix] Separate XDaiForeignBridge contract with compound and GSN support (#626)
 * [Fix] Update GSN interface (#628)
 * [Fix] Block ERC20 selectors in AMB requests (#630)
 * [Other] Bump package and contracts interfaces version prior to 6.0.0 (#629)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit Items related to the security audits team-august-2021 Related to items found by ChainSecurity in the audit held in August of 2021
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants