See foundry-rs/foundry#4826 (comment).

Problem

One of the biggest pain points of using Permit2 with Safe is that all signatories need to sign the permit in a round-robin process.

We had a call with Utopia today to discuss a potential integration, and they said that this is one of the biggest issues with our current periphery setup.

Solution

Allow the periphery to be used via standard ERC-20 approvals.

Proposal

We either:

1. Extend the current ProxyTarget to accept standard ERC-20 approvals
2. Turn the current ProxyTarget into an abstract with shared logic, and build two other ProxyTargetPermit2 and ProxyTargetERC20 to implement the Permit2 and ERC-20 variants.

The CI workflows in #31 have failed because of the test_BatchCancelMultiple test.

I did a little bit of investigation, and the issue is related to V2 Core, specifically the precompiled version of it. Switching to normal deployments makes the tests pass again.

See OpenZeppelin/openzeppelin-contracts#4231 and the release notes for OpenZeppelin's v4.9.0-rc.0.

We need to change all OpenZeppelin imports to be @openzeppelin/contracts instead of @openzeppelin because the Node.js package @openzeppelin/contracts cannot be remapped.

Running FOUNDRY_PROFILE=optimized forge build generates contract artifacts in the optimized-out directory, many of which should not be part of the Node.js package that gets deployed on the npm registry.

We should write a script that filters Forge's output and keeps only those contracts that are not needed in production (e.g. not the scripts).

Context:

• #143
• solhint-community/solhint-community#25

As per the rationale shared in #59.

Problem

As pointed out by @andreivladbrg in this comment, and as further explained by me in this reply, there is a low-risk security problem in our current proxy plugin implementation.

The problem is that anyone can call the onStreamCanceled implementation on the plugin, like this:

uint128 senderAmount = 10e18;
uint128 recipientAmount = 0;
ISablierV2LockupSender(address(proxy)).onStreamCanceled(lockupContract, streamId, recipient, senderAmount, recipientAmount);

And thus, anyone can withdraw any amount of ERC-20 assets from the proxy contract to the proxy owner:

https://github.com/sablierhq/v2-periphery/blob/e62b768/src/SablierV2ProxyPlugin.sol#L53

This is a low-risk issue because, generally, there is no incentive to trigger random ERC-20 transfers on behalf of other accounts, but there may be cases when this is an undesirable state of affairs:

1. Adversarial contexts. The proxy contract may perform a flash loan, and a competing flash borrower who sees their tx in the mempool could block it by withdrawing the assets to their EOA.
2. Griefing: deliberately blocking someone's proxy contract transactions for no particular benefit.
3. Tax implications. Withdrawing assets from the proxy contract might be interpreted as a taxable event.

Potential Solution

The lockup address is a user-provided function parameter:

https://github.com/sablierhq/v2-periphery/blob/e62b768026d5df52a3df340cd0df108bf455629e/src/SablierV2ProxyPlugin.sol#L37

This makes it difficult to mitigate the problem. The only solution I can think of now is to add an allowlist of Sablier contract addresses in SablierV2ProxyPlugin and cross-check lockup against it. However, implementing this would also require us to inherit from Adminable and write an admin-gated function for listing new contracts, i.e., addSablierContract, which would be demanding.

See:

• foundry-rs/foundry#4833
• https://github.com/foundry-rs/forge-std/releases/tag/v1.5.4?utm_source=substack&utm_medium=email

We have to bump PRBTest and switch to using the new expectCall overload with a count parameter.

Tracking:

• batchCreate functions
• cancelAndCreate
• cancelMultiple
• onStreamCanceled
• wrapAndCreate

@andreivladbrg started working on this on the andrei/fork-tests branch.

The fork tests are currently run against a single chain.

Ref: foundry-rs/book#872

Note: all caveats mentioned here still apply to forge coverage.

• Generate coverage on every commit and PR on main
• Upload coverage report to a platform like Codecov

Just like sablier-labs/v2-core#372.

To be able to publish a functional Node.js package, we need to install the following Node.js dependencies, and list them in package.json:

• @openzeppelin/contracts
• @prb/math
• @prb/proxy
• permit2

Blockers:

• PRBProxy v4 is not on npm just yet (I'll fix this shortly)
• Permit2 is not on npm Uniswap/permit2#216 edit: not a blocker, we can install NPM packages from GitHub

As the recent saga with the Permit2 signatures in the front-end has shown, it would be a good idea to delete the hard-coded Permit2 address, and make it a user-provided parameter.

As discussed in #54, we should get rid of the requirement to keep the Permit2 params in synchrony by replacing our Permit2Params struct with PermitSingle:

https://github.com/Uniswap/permit2/blob/bbbc92f895049ca45c4b25a450f9d3e907659284/src/interfaces/IAllowanceTransfer.sol#L55-L62

This way, the front-end client will have to manually pass the PermitSingle struct to the ProxyTarget contract.

Which contains the fix for sablier-labs/v2-core#616

Same as sablier-labs/v2-core#483.

Analogous to sablier-labs/v2-core#445.

In the latest PR we added re-export types. However, this does not work for permit2 due to the remappings we declared:

When attempting to import the permit2 types from v2-periphery, the following error will appears:

The solution to this issue involves changing the remappings to:

@permit2/=lib/permit2/src/
@permit2-test/=lib/permit2/test/

We are currently using the try/catch statement to call the create functions from the core contracts.

This is a problem because we are transferring a total amount of assets (user --> periphery) before making the external call

https://github.com/sablierhq/v2-periphery/blob/180d5dc194c30cc95db9578a18b9db3b9598300c/src/BatchStream.sol#L94-L95

If a stream creation fails(create multiple will not revert) the amount that should have been transferred (periphery --> core) will remain in our periphery contract.

Same as sablier-labs/v2-core#200.

@andreivladbrg, since you handled our Permit2 integration, you have context. It would be nice to document each field in the Permit2Params:

https://github.com/sablierhq/v2-periphery/blob/abc39da26be9a3db49266770494dc50fba41570e/src/types/DataTypes.sol#L57-L63

Just like we do with all structs in V2 Core:

https://github.com/sablierhq/v2-core/blob/6362f84642debc4d48109605db800e54163e726b/src/types/DataTypes.sol

Just like we do in ProxyPlugin, we should inherit from the OnlyDelegateCall abstract in ProxyTarget to prevent accidental direct calls to ProxyTarget, and then apply the onlyDelegateCall modifier to all user-facing functions.

The nested mappings here are overkill:

• It's bad UX to have to provide so many inputs to query a user's campaigns
• There's no benefit in splitting the campaigns by type, i.e., LockupLinear and LockupDynamic. Users are interested in all of their airstream campaigns; the payment type is a low-level detail.
• There's no point in storing the campaign details (expiration, Merkle tree) in the factory. What @razgraf suggested on the call was to emit these data in an event, not store them.
• Should there be a need to obtain the campaign's details (expiration, Merkle tree) those can be queried from the subgraph or the Airstream contract itself.
• Should there be a need for advanced filtering, the subgraph can help with that.

Given the feedback above, the solution is to rewrite the mappings like this:

mapping(address user => ISablierV2Airstream[] contracts) internal _airstreams;

Let's please use internal over private. It would make testing easier.

Further corollaries:

• There's no point in blocking users from creating similar campaigns, i.e., we can remove the CampaignAlreadyDeployed error and the associated checks in the createAirstream functions. The same Merkle tree can be used for multiple airdrops.
• We can remove the getAirstreamLockupDynamic and getAirstreamLockupLinear getters, and replace them with a new getter getAirstreams, which returns all of the user's airstream campaigns.

Smth like:

import { IPRBProxy } from "@prb/proxy/interfaces/IPRBProxy.sol";

function foo() external {
    address owner = IPRBProxy(address(this)).owner();
}

Refs:

• PaulRBerg/prb-proxy#108
• https://github.com/cantinasec/review-sablier2/issues/2

• Bump @prb/proxy to stable v4.0.0 (ref PaulRBerg/prb-proxy#126)
• Double-check README and wikis
• Make the repo public
  • Remove the Codecov token
• Regenerate gas snapshot
• Run deep fuzzing in CI

As of recently, the v2-periphery makes use of a SablierV2Archive registry contract to track Sablier deployment.
For a more seamless deployment experience we should attach deployment scrips to v2-periphery that can handle both v2-core instantiation and instance registration in the archive contract.

Benefits:

1. Prevent malicious envoys from misusing SablierV2ProxyTarget
2. Less error-prone because there's no reliance on the front-end to pass the correct address

Ref https://github.com/cantinasec/review-sablier2/issues/3.

Problem

While working on the proxy plugin, we stumbled upon the issue described in #36.

Thinking about a solution, I have discovered another problem - that we offer zero on-chain support to integrators in terms of the ability to fetch the Sablier V2 contract addresses in another smart contract.

Solution

To address this problem, as well as fix #36, we should build something similar to Maker's ChainLog contract. The documentation for Maker's contract can be found here.

As discussed here, to handle cancellations triggered by recipients, we need to implement a proxy plugin for the onStreamCanceled hook. In this scenario, the funds would be withdrawn to the proxy, and we want to automatically redirect them to the sender.

For context, see IPRBProxyPlugin.

• Add "Notes" in cancel to explain that assets are automatically refunded
• Explain in the proxy plugin NatSpec that the Core hooks are what make this design possible
• Remove the requirements here (they are superfluous)

Install the latest @sablier/v2-core, and then update the import paths to use src/.

Refs:

• sablier-labs/v2-core#646
• sablier-labs/v2-core#648

Analogous to CONTRIBUTING.md in V2 Core.

Once V2 Core is published and deployed to Ethereum Mainnet, we should refactor the fork tests in this repo to use Ethereum Mainnet instead of Goerli.

Related:

• sablier-labs/v2-core#577
• #110

This is an end-to-end test to ensure that the Periphery system is coupled correctly.

We need to find a new name for the SablierV2ChainLog contract:

https://github.com/sablierhq/v2-periphery/blob/main/src/SablierV2ChainLog.sol

Because this is the same name as used by Maker's dss-change-log, which is a versioned chain log system. Whereas our ChainLog is an unversioned "dump" of all Sablier addresses (see my comment here).

Given that someday we may want to implement a similar versioned log, it would be prudent to avoid a potential name collision by renaming the current ChainLog to something else.

The goal is to not require end users to install third-party packages.

See Tokens.sol and Math.sol in V2 Core.

See:

• cancel
• renounce
• withdraw
• withdrawMax

In the current form we can suport the withdraw action for both the sender and the recipient, but the withdrawMultiple functionality can be implemented just for recipients. If we add withdrawMultiple in the target we can implement withdrawMultiple for senders as well.

Refs:

• https://github.com/solhint-community/solhint-community
• PaulRBerg/foundry-template#25
• https://twitter.com/PaulRBerg/status/1666753482609553408

I removed the batchCreate fork tests from #73 due to the pressure to wrap up the repo for the audit. These tests were not working anymore, and I did not have the time to review them properly.

Here's a GitHub Gist with @andreivladbrg's code:

https://gist.github.com/PaulRBerg/156d2726b28e54510f89d9fdb99a294a

We're now running the fork tests against the Mainnet deployments, so the id of the first stream may not be 1 anymore.

Find a way to re-use GitHub workflows between this repository (V2 Periphery) and other similar repositories such as V2 Core.

For example, we could write a reusable workflow for the lint and the build jobs.

We can replace our local getPermit2Signature helper with the utils in permit2/test/utils/PermitSignature.sol.

Once we publish @sablier/v2-core as a package to the npm registry, we should install it as a Node.js dependency.

Same rationale as in #29 (comment)

• Inherit from PRBProxyStorage in ProxyTarget
• Use owner instead of msg.sender

Once we make this repo public, we will want to pass the --verify flag to all deployment scripts under .github/workflows so that their source code gets verified automatically on Etherscan.