Skip to content
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

use call - not transfer - to enable Gnosis Safes #72

Merged
merged 4 commits into from
Aug 4, 2024
Merged

Conversation

anna-carroll
Copy link
Contributor

@anna-carroll anna-carroll commented Aug 1, 2024

Motivation

2300 gas forwarded by transfer is not enough for Gnosis Safes' receive function

Description

Using OpenZeppelin's Address library, switch to using call, not transfer, to send Ether.

Reentrancy

  • added explicit reentrancy protection in fill, sweep, and withdraw.
    • previously, this was not strictly unnecessary because transfer did not provide enough gas to reenter the sensitive functions such as fill.
  • for consistency, also added explicit reentrancy protection to every other fn that calls un-trusted external contracts - fillPermit2, initiate/initiatePermit2, enter/enterPermit2, exit/exitPermit2.

Gas Cap

Previous iteration of this PR included a gas cap on ETH transfers.

Have now opted to provide un-capped gas for all ETH transfers (in tandem with reentrancy protection).

In the case of fill or withdraw, this is something to be mindful of -

  1. Fillers pay gas for fill, though swappers provide the recipient
  2. tokenAdmin pays gas for withdraw, though exit-ers provide the recipient
    In these cases, important to be mindful of outsized gas consumption by a smart contract recipient.

In the case of attempted malicious gas consumption,

  1. Fillers can simply opt not to fill the Order in question.
  2. tokenAdmin can refuse to provide service to a malicious withdrawer.

If there's an attempt to front-run a Filler's fill transaction and deploy a contract at the recipient address in order to try to steal gas tokens or grief the Filler, the Filler's fill transaction should revert due to insufficient gas. Their transaction simulation would have provided an overall gas limit for the transaction, which would run out if the recipient suddenly consumes more gas. I believe there is no way to steal Fillers' funds through this process - if the fill call reverts then the initiate transaction will also not process, the sweep call will revert, and the only thing lost is the gas for the reverting transactions.

@anna-carroll anna-carroll self-assigned this Aug 1, 2024
@@ -125,7 +128,8 @@ contract Passage is PassagePermit2 {
function withdraw(address token, address recipient, uint256 amount) external {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be non-reentrant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per description, since it's only callable by the tokenAdmin i don't think we need to worry about reentrancy

@prestwich prestwich merged commit ffae635 into main Aug 4, 2024
1 check passed
@anna-carroll anna-carroll deleted the anna/use-call branch August 5, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants