Jump to content
JoelKatz

An update on Ripple's XRP escrow

Recommended Posts

For the record, I'd say that there were two oversights here that came together to make the (small) problem with the escrows.

The first problem is with the design of the Escrow feature. When creating an escrow, you have to specify either FinishAfter or CancelAfter and you can optionally specify a Condition too. The thought was, you can do a time-based escrow, or a condition-based escrow, and if it's condition-based, you should have a cancellation deadline. Even though we documented the implementation correctly, it wasn't raised as a problem that you can create what I think of as "false escrows" (which can be finished by anyone at anytime between the creation and cancellation) by using CancelAfter with neither of the other two fields. We tested many cases of what you should and shouldn't be able to do with escrow when using it correctly, but I don't think it really sunk in to anyone at Ripple that, as implemented, you can use the feature incorrectly in such a way that it looks pretty similar to using it correctly.

The second problem was when creating and signing the actual EscrowCreate transactions, we accidentally used the CancelAfter field instead of FinishAfter. We knew that they should use FinishAfter but when creating the actual transactions we missed it. That meant that we fell into this trap—having created escrow objects in the ledger with an expiration date on them and everything, except that there was no condition locking up their release. A truly embarrassing oversight, to be honest. Fortunately, we recognized the mistake shortly after creating the transactions, and were able to create new transactions using the correct field thereafter.

Moving forward, I believe we can address the first problem by amending the protocol to require either FinishAfter or Condition on every escrow. I expect we'll do that as part of our normal development cycle, so that the changes aren't rushed and have time to go through rippled's famously thorough code review process. It may also be worth revisiting the actual design process, which I think this shows is not nearly as bulletproof as our code review process. That's somewhat outside my area, though.

I'm not privy to the signing ceremony involved in the second problem, but I'm sure that the people who are will implement new checks in the process to prevent this kind of oversight again.

Share this post


Link to post
Share on other sites
On 12/18/2017 at 2:27 PM, mDuo13 said:

For the record, I'd say that there were two oversights here that came together to make the (small) problem with the escrows.

The first problem is with the design of the Escrow feature. When creating an escrow, you have to specify either FinishAfter or CancelAfter and you can optionally specify a Condition too. The thought was, you can do a time-based escrow, or a condition-based escrow, and if it's condition-based, you should have a cancellation deadline. Even though we documented the implementation correctly, it wasn't raised as a problem that you can create what I think of as "false escrows" (which can be finished by anyone at anytime between the creation and cancellation) by using CancelAfter with neither of the other two fields. We tested many cases of what you should and shouldn't be able to do with escrow when using it correctly, but I don't think it really sunk in to anyone at Ripple that, as implemented, you can use the feature incorrectly in such a way that it looks pretty similar to using it correctly.

The second problem was when creating and signing the actual EscrowCreate transactions, we accidentally used the CancelAfter field instead of FinishAfter. We knew that they should use FinishAfter but when creating the actual transactions we missed it. That meant that we fell into this trap—having created escrow objects in the ledger with an expiration date on them and everything, except that there was no condition locking up their release. A truly embarrassing oversight, to be honest. Fortunately, we recognized the mistake shortly after creating the transactions, and were able to create new transactions using the correct field thereafter.

Moving forward, I believe we can address the first problem by amending the protocol to require either FinishAfter or Condition on every escrow. I expect we'll do that as part of our normal development cycle, so that the changes aren't rushed and have time to go through rippled's famously thorough code review process. It may also be worth revisiting the actual design process, which I think this shows is not nearly as bulletproof as our code review process. That's somewhat outside my area, though.

I'm not privy to the signing ceremony involved in the second problem, but I'm sure that the people who are will implement new checks in the process to prevent this kind of oversight again.

I wouldn't want to be the person who pushed Enter on the first lockup attempt. I bet they are banned from the fresh muffins in the break room forever more.

Can you imagine the first thought that went through that persons head when a 'mistake' was found? 'OMG... I... I... I just lost all the XRP...'

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×
×
  • Create New...