Design of the "price gated timelock" model

[andreivladbrg]

Problem

We don't have a consensus on the design here: #1280

Solution

TBD

---

Currently, we are using a Chainlink oracle for the minimum fee calculation in USD, and we don't even have a price feed for the native tokens of the chains we are deployed on, which means that the tokens supported by this new model would be limited. See the ones available here.

In order to implement the "price-gated timelocks" model, we would need to use oracles as well.
@smol-ninja mentioned in the issue above that the oracle addresses can be passed as input to the create function, as there is an assumption that the parties (sender & recipient) trust each other.

This option, IMO, brings security considerations to us as well. The main reason is that we would have an early (before any state update) "interaction" operation during the cancel and withdraw functions with a contract that we don't know about beforehand. For example, consider the following scenario with a malicious oracle:

• cancel is called and the streamedAmountOf function calls the oracle to get the price
• the malicious oracle implementation first calls the withdraw function and then its latestRoundPrice function
• withdraw also calls streamedAmountOf, which calls the oracle to get the price
• the malicious oracle knows that the second call is being made through the withdraw function and returns a price greater than the target price, allowing to fully withdraw the deposited amount
  • the malicious oracle can infer the call sequence using a state variable set to specific values during execution, similar to non-reentrant logic
• the withdraw execution passes and transfers tokens out of the lockup contract
• now latestRoundPrice - in the initial cancel function call - returns a price lower than the target price, making the refundable amount greater than 0, thus transfers more tokens out of the lockup contract than deposited

The scenario above may be fixed by disallowing cancelable streams for the price-gated model and adding nonReentrant to the withdraw function, but the idea of having an interaction with a contract that we do not explicitly "allow" is a no-go, IMO.

The version I suggested, which adds medium to high maintenance overhead, is to have an allowlist system for oracles.

@sablier-labs/evm let me know what you think

[smol-ninja]

Thanks Andrei for posting the discussion.

the tokens supported by this new model would be limited. See the ones available here

I reckon your assumption that through allowlist, we will be managing a few tokens per chain. Chainlink offers support for multiple tokens on each network. For ex. on Arbitrum, Chainlink supports 500 tokens. If its between managing 10,000s of allowlist over isolating risk to a single stream, I would vote for isolating risk for a single stream. And I think through our UI, both senders and recipients will be able to detect if the provided oracle is a valid Chainlink address or not.

As for the design, there is no streamedAmountOf function here. During cancel and withdraw, we will just check of the oracle price > target price or not and unlock / cancel the full amount. The sequence of calls you provided will not work if we have nonReentrant modifier to both withdraw and cancel (as you have suggested as well).

You are correct that withdraw and cancel will be making a call to a user-provided oracle address but in any case, the risk is only isolated to that particular stream and will not let stream recipient withdraw more than the deposited amount.

Let me know what you think now.

[PaulRBerg]

applying the same logic, what’s the point of not making them (hooks) permissionless?

Again, different problem situation.

We had to make the hooks permissioned because of gas bombs issues.

And yes, there is trust required for them

[andreivladbrg]

saying this at the start: we will implement the permissionless system. that's the business decision.
i continued this thread for two reasons: (i) security of the smart contracts and (ii) technical decisions of the smart contracts (this is the lockup repo, not command center), with the intention of taking good decisions in the implementation

no it's not relevant because the Sablier UI ultimately decides the fee, not the contract. if the contract min fee is too high, then the tx won't go through

you are correct with the fact that UI decides the ETH value sent in the withdraw function which must >= min fee calculated onchain

think of these scenarios:

1. the native token oracle configured is incorrect - recipients is required to pay a very high fee, thus he can't withdraw
2. the oracle set for a given token is set incorrect - thus recipient won't get his token unlocked when the price actually is reached that moment, thus recipients can't withdraw

the result of the recipient not being able to withdraw is similar - but in case of the permisionless the sender is responsible, for the allow system we would be

and we will fix it

we can fix oracles addresses for both functionalities

@andreivladbrg please try to stay on topic.

i believe i am on topic: smart contract design of the price-gated functionality, which requires the use of oracles. what was not related to this?

but if our recommended oracle in the escrowed bets contains errors, then users lose a lot of money

no, the users won't lose money, but be temporarily unable to withdraw (we can fix it)

We had to make the hooks permissioned because of gas bombs issues.

a gas bomb can happen here as well. the sender can set an oracle address that would force the recipient to pay a high fee

And yes, there is trust required for them

the difference with hooks is that we can review the contract first to see whether a gas bomb can happen or not

---

@smol-ninja curious about your opinion regarding the similarities between the allowlist for hooks and this feature.
from my perspective, the differences are who gets gas-bombed (sender vs recipient) and who provides the funds:

• oracle gas bomb: sender (who funded the stream) sets the oracle -> recipient faces high costs for withdrawal
• hook gas bomb: recipient transfers the nft to that contract -> sender faces high costs for canceling the stream

[PaulRBerg]

@andreivladbrg let's discuss in person or over a call or in a separate issue/discussion. you've went off-topic

[smol-ninja]

how is this different from the allow system for running the hooks at the end of cancel & withdraw?

A major issue with permissionless hooks was that a recipient could transfer the stream to a contract and cause a denial of service that wasn’t anticipated when the stream was created. However in case of timelock, pool depositors can evaluate the oracle's reliability at the time of deposit or the recipient can consult the sender about any oracle concerns. Since the oracle is known in advance, the associated risk is known from the beginning and persists throughout. These are different scenarios imo.

a gas bomb can happen here as well. the sender can set an oracle address that would force the recipient to pay a high fee

This is also different here. In case of hooks, the maximum gas usage isn’t predictable because a hook can execute any arbitrary valid logic, which might consume significant gas causing gas bomb griefs. In case of oracle feed, we can define a maximum gas limit for the staticcall (which would be very low) making sure that gas bomb attacks are not possible.

[andreivladbrg]

good points Shub

the fact that it is a "view" function (using staticcall) is an important detail in case of vulnerabilities

going to close the discussion