liquity / dev

Liquity monorepo containing the contracts, SDK and Dev UI frontend.
GNU General Public License v3.0
318 stars 308 forks source link

Improve recovery from outdated hints in SortedTroves #600

Open danielattilasimon opened 3 years ago

danielattilasimon commented 3 years ago

The current version of SortedTroves could be further improved for the case when hints become outdated because the NICR of the inserted Trove increases over time. This is a fairly common occurance due to the decay of the borrowing fee.

Let’s say we're trying to create a new Trove that has a lower ICR than the current lowest ICR (i.e. inserting into the tail of the list). findInsertPosition() returns (addressOfLowestICRTrove, address(0)) in this case. If we blindly pass these as hints to openTrove() (as we currently do in the middleware), but the transaction ends up pending for so long that we need to look for a new insert position, we start the traversal in the wrong direction (from the wrong end of the list), and end up having to traverse over almost the entire list.

danielattilasimon commented 3 years ago

Workaround for the current version is to pass (addressOfLowestICRTrove, addressOfLowestICRTrove) as hints.

RickGriff commented 3 years ago

We just had a user who had the lowest ICR trove in the system, trying to adjust his trove to go lower. The tx failed before broadcast - presumably because gas estimation didn't allow it.

I believe this is the edge case DeFiSaver found - and I mistakenly thought the solution outlined above also covered it. The problem is that for the last trove in the list, the addressOfLowestICRTrove is their own address - and the reinsertion first removes it before the insert. The system can't find it in the list, so starts at the opposite end, and runs out of gas.

The DefiSaver fix was to check for this specific edge case: if the trove is last in the list, use the 2nd last trove's address as both hints.