-
Notifications
You must be signed in to change notification settings - Fork 8
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
Spike: Payment channel authorization grants #36 #41
Conversation
Keypair is saved with encryption key in deployments
|
||
// fund the new sub channel with the deducted amount from the main channel | ||
subChannel.investedByPublisher += amount; | ||
subChannel.unlockTime = channel.unlockTime; // Inherit unlock time from main channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading the code right, this lets the publisher instantly unlock any channel, by doing (in all below calls, msg.sender == publisher
):
0. createChannel(provider, podId, ...)
(created earlier)
createChannel(publisher, 0x0, 0, 1)
(create a dummy channel with unlock time 0)authorize(publisher, publisher, 0x0)
(authorize someone for it (in this case, ourselves))createSubChannel(publisher, publisher, 0x0, provider, podId)
(oops! resets unlockTime on the original channel to 0)unlock(publisher, provider, podId)
(regular unlock)withdrawUnlocked(publisher, provider, podId)
(oops! can withdraw supposedly-locked funds without waiting for unlockTime) (would work even if made in the same block as all the other commands, with no chance for the provider to cash out any work done since the lastwithdraw(..)
call)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, Should be Fixed now.
if (channel.investedByPublisher < amount) revert InsufficientFunds(); | ||
|
||
// Deduct the amount from the main channel's funds | ||
channel.investedByPublisher -= amount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decreasing the amount invested by the publisher without requiring the channel to be unlocked first allows the publisher to drain the channel of any funds within it, by creating a subchannel with all the funds they can, then unlocking that subchannel, and withdrawing from it at a leisurely pace, without the provider able to submit a withdraw()
call.
Also, since the code never checks whether channel.investedByPublisher - channel.withdrawnByProvider < amount
(see, e.g. line 121, leftoverFunds = ..
), it's possible to withdraw more than was initially invested into a channel, thus draining the whole contract of funds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed as well
This pull request attempts to fix payment issue #36
Smart Contract
To run test you can run the following command :
forge test --root ./contracts
Publisher Updates
--authorize
flag to the deploy command which will create an asymmetric key pair.Provider Updates
Autoscaler
PaymentChanneManger
Testing
Proto files have been updated, sync the new generated files with
turbo sync
You can re-run the e2e test in
test/e2e/autoscaler
.If the test has already been run before, you can restart the clusters (if they are not already running) with
start-clusters.sh
. Then, redeploy the tpodserver and autoscaler usingredeploy-images.sh
. After that, skip to deploying the autoscaler withrun-test 5.0
.