Skip to content
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

wip: LUSD Chicken Bonds Implementation #21

Merged
merged 39 commits into from
Apr 13, 2022
Merged

wip: LUSD Chicken Bonds Implementation #21

merged 39 commits into from
Apr 13, 2022

Conversation

RickGriff
Copy link
Contributor

@RickGriff RickGriff commented Feb 21, 2022

(PR merged and task list deprecated 13/04/2022 - See main ChickenBonds repo Readme for current task tracking)

Running the project

ChickenBonds is a Foundry project in /LUSDChickenBonds.

Install Foundry:
https://github.com/gakonst/foundry
Run tests with forge test.

Core contracts are found in src.

TODO List

Initialization

  • 50:50 split upon the very first chicken-in to initialize the bTKN/TKN pool.
  • Market price = redemption price = 1
  • Forbid chicken-ins during 1 week (tbd)
  • Forbid redemptions during 1 month (tbd)

Base functionality

  • BondNFT

  • sLUSDToken
  • mock Curve pool
  • mock Yearn LUSD and Curve vaults
  • createBond
  • chickenIn

  • chickenOut
  • redeem

  • Change accrual function to asymptotic curve, remove cap constraint
  • refund functionality inside chickenIn
  • Shifting functions
  • Replace refund functionality with permanent bucket
  • Liquity-like redemption fee
  • Extract common functionality in core contracts
  • Extract common setup functionality in unit tests
  • Basic math function for converting to/from 18 digit fractions
  • Implement main events
  • Settle on best Solidity version to use (OZ contracts are v8+, and Slither detects v8+)
  • Add return values to all state-changing functions for integrations
  • Tax on chicken in + incentive for bTKN/TKN pool
  • Adapt shifting function: revert when Curve price crosses boundary

External contracts integrations


  • Determine most accurate way to compute totalAccruedLUSD from Yearn and Curve
  • Determine Curve trade quantity calculation for shifting functions (we simply revert if the price crosses the $1.0 boundary)
  • Implement Yearn Registry check for latest vaults
 and migration functionality
  • Connect to real Yearn and Curve contracts
  • Mainnet hard fork testing



Security

  • Create thorough unit test plan: negative tests, multiple bonds per user, etc.

  • Create list of system properties/invariants and add asserts
  • More extensive testing for edge cases (e.g. pending harvests, harvest losses, external calls to Yearn/Curve reverting, etc)
  • Run Slither / MythX
  • Systemic fuzzing (Dani?)

Design

  • Determine redemption fee formula

  • Determine sLUSD accrual function
 / controller
  • Decide on NFT enumeration (i.e. getting all of a user's bonds), and/or extra trade functionality

import "../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import "./console.sol";

contract SLUSDToken is ERC20, Ownable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we planning to blacklist some contract addresses to prevent accidental token transfers from causing token losses?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - we could blacklist the core ChickenBonds contract and token addresses, as we did with Liquity.

- Decide whether we need an on-chain function for listing all bonds owned by the user, or whether events are sufficient
*/
contract BondNFT is ERC721, Ownable {
using Counters for Counters.Counter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of using an external counter function? Is it more secure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the OpenZeppelin Counter template. Very minimalistic:
https://github.com/liquity/ChickenBond/blob/lusd_chicken_bonds/LUSDChickenBonds/lib/openzeppelin-contracts/contracts/utils/Counters.sol
All it really does (beyond counting) is require that the counter never goes below 0 - it's a good check to have for security.

Copy link
Contributor

@cvalkan cvalkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments and questions.

yearnLUSDVault.withdraw(yTokensToBurn);

// Send bonded LUSD back to caller and burn their bond NFT
lusdToken.transfer(msg.sender, bondedLUSD);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If msg.sender is a contract and the call reverts, could that lead to issues, i.e. LUSD lost in transition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it reverts, the the whole transaction is undone and no LUSD would be sent. As a basic ERC20 token transfer, the only way this line would revert is if there's insufficient LUSD balance to send.

Also, IMO this transfer should never revert anyway i.e. it's a system invariant that it always succeeds. The exception is if there's a Yearn or Stability Pool hack. The ChickenBonds system should otherwise always have enough pending LUSD to cover all chickenOuts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a basic ERC20 token transfer, the only way this line would revert is if there's insufficient LUSD balance to send.

I thought it could revert in case of a custom fallback function. But I don't think that this causes a threat at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that’s only for ETH transactions, not for ERC-20

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right - why do I keep forgetting that. :)

totalPendingLUSD -= bondedLUSD;

uint yTokensToBurn = yearnLUSDVault.calcYTokenToToken(bondedLUSD);
yearnLUSDVault.withdraw(yTokensToBurn);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there cases where the actual withdrawal from Yearn would revert? What would be the consequences?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially, yes. Good question. The Yearn withdraw function looks quite unrestricted, and should only revert at the top level if there's some critical bug in the Yearn vault. I wil dig further into the underlying strategy contract that it's connected to as well.

In contrast, the deposit function would revert if the Yearn vault is in emergency mode, but withdrawals are always allowed.

In the worst case we can make sure a call doesn't revert the whole transaction with a try-catch, but we'd need think about how to proceed if we can't obtain the expected tokens.


// Calculate redemption fraction to withdraw, given that we leave the fee inside the system
uint fractionOfAcquiredLUSDToWithdraw = fractionOfSLUSDToRedeem * (1e18 - calcRedemptionFeePercentage()) / 1e18;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the yearnLUSDVault.balanceOf(...) return the entire balance held in the vault (including the pending bonds) rather than just the acquired part? Wouldn't we need to call getTotalAcquiredLUSD() here?

Also, what happens when the very first bonder chickens in and there's already some accumulated compounded yield generated by the bonded LUSD? If it just keeps accumulating and becomes redeemable, then the first one who chickens in could have an incentive to redeem all his sLUSD right away, which may not be ideal.

Copy link
Contributor Author

@RickGriff RickGriff Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - we need to ignore the pending portion in the redemption.

Re: early redemptions, yes agreed. As we discussed earlier on Slack (for other readers):

Rick: Generally it seems like redemptions may be tempting as long as there is no robust sLUSD market. Perhaps we need to prevent and/or limit redemptions early on, and then gradually phase them in over time to allow an sLUSD market to develop.

Robert: We definitely should do that. Initially, I though we just need a fixed time period starting from the first chicken-in event, but perhaps we need to be more restrictive. If we have a redemption fee, we could maybe bake that into the formula somehow, or take the total sLQTY supply into account.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still trying to wrap my head around this and see whether the issue is just due to the lack of a robust sLUSD market or more systemic.

Whenever somebody chickens in, we need to first determine the pending yield on the acquired and pending buckets in order to calculate the cap and thus accruedLUSD. It can happen that somebody who bonded x LUSD would get >x LUSD when redeeming the received sLUSD.

At first sight this looks like an issue. But if users are confident that they will be able to sell or redeem the sLUSD for more in the (near) future, it's perhaps fine. As long as there are plenty of outstanding bonds (and the yield amplification factor is high), there's no apparent reason to redeem sLUSD even if there exists no liquid sLUSD/LUSD market yet. Why redeem now if you can get an amplified yield by redeeming later (and potentially even more when a liquid sLUSD market becomes available)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can happen that somebody who bonded x LUSD would get >x LUSD when redeeming the received sLUSD.

Do you mean immediately? With the original definition of backing ratio that shouldn’t be possible. With the new definition suggested by @danielattilasimon (“a variable that starts at a constant (of our choosing) and is updated every time yield is harvested, by multiplying it by the yield”), I guess it shouldn’t either if we choose the initial value right.

Anyway, I guess we should make sure rewards and yields are collected both before chicken in and redemption. Do these withdraw functions in Yearn Vaults collect them too?

Copy link
Contributor

@cvalkan cvalkan Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean immediately? With the original definition of backing ratio that shouldn’t be possible.

Yes. Why wouldn't it be possible. If there are large pending bonds generating yield before the first chicken in, the Acquired Bucket could be larger than the bond of the very first bonder who chickens in. It doesn't matter how much sLUSD this chicken gets, since it would redeem for the entire Acquired Bucket which is larger than the amount of LUSD bonded by the chicken.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, you are right, it’s possible for the first chicken in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear it's also possible for subsequent chicken-ins if the first chicken redeems the entire sLQTY supply.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I should’ve phrased as “it’s possible when sLUSD total supply is zero”. In a way, that subsequent chicken in would be first too, 😉

Copy link
Contributor

@bingen bingen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive progress!! I’m still reviewing it, but leaving some minor comments below.
I’m afraid I have to catch up with some things first, like Solidity 0.8 (so weird and nice to see all those operations without SafeMath, haha), Yearn, NFTs...

out/
lib/
.idea
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can .gitignore .DS_Store too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And solc binary too.

- Decide whether we need extra functionality for OpenSea / other markets, e.g. baseTokenURI
- Decide whether we need an on-chain function for listing all bonds owned by the user, or whether events are sufficient
*/
contract BondNFT is ERC721, Ownable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not too familiar with NFTs yet, it seems to me that we don’t need ERC-1155 extra functionality, but I wanted to know your thoughts about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, I'm still getting up to speed on NFTs. I wanted to begin with the most minimalistic implementation.

Agreed, I don't think we need the extra ERC1155 functionality but I'll look into it further.

_tokenSupply.increment();

uint256 tokenID = _tokenSupply.current();
_safeMint(_bonder, tokenID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work with a DSProxy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. From what I can see, the DSProxy will own the NFT and could send it to anyone else if the owner wanted.

Copy link
Contributor

@bingen bingen Apr 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the documentation header for ERC721 says:

If `to` refers to a smart contract, it must implement {IERC721Receiver-onERC721Received}, which is called upon a safe transfer.

So DSProxy wouldn’t implement it, right?

// SPDX-License-Identifier: MIT
pragma solidity >=0.4.22 <0.9.0;

library console {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were using ds-test logs instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to do so - the HardHat style console.log is optional, and the Foundry team added it in


contract ChickenBondManager is Ownable {
// ChickenBonds contracts
IBondNFT public bondNFT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make these ones immutable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS, yes - good idea.



// TODO: Decide between one-time infinite LUSD approval to Yearn and Curve (lower gas cost per user tx, less secure)
// or limited approval at each bonder action (higher gas cost per user tx, more secure)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I’ll think more about it.
On one hand, all our contracts at Liquity work with implicit approvals. Besides, how would we protect subsequent approvals?
On the other hand, “less secure” sounds scary and we approving to external contracts. Are Yearn vaults upgradeable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, Liquity has implicit infinite approval for core system contracts. In this case they're somewhat untrusted external contracts so we should perhaps be more careful.

Yearn vaults are upgradeable, shutdown-able, etc. They have a fair bit of admin control.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, individual, finite approvals before each call seems like the safer bet. Otherwise, the 3rd party may upgrade their contracts, adding a function to sweep LUSD from our contract.

BondData memory bond = idToBondData[_bondID];
uint accruedLUSD = _calcAccruedSLUSD(bond);

_requireCapGreaterThanAccruedSLUSD(accruedLUSD, bond.lusdAmount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won’t be needed with the asymptotic accrual, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it is, then maybe we can cache bond.lusdAmount.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right. It's a TODO to change this to an asymptotic curve of the form t/t+r, so we can get the accrual logic closer to the final form.

yearnCurveVault.withdraw(yTokensToWithdrawFromCurveVault); // obtain LUSD3CRV from Yearn

uint LUSD3CRVDelta = curvePool.balanceOf(address(this)) - LUSD3CRVBalanceBefore;
curvePool.remove_liquidity(LUSD3CRVDelta); // obtain LUSD from Curve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t we use the single-sided one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when we change this from the mock Curve pool to the real one we should


/* Get LUSD amounts to acquire and refund. Acquire LUSD in proportion to the system's current backing ratio,
* in order to maintain said ratio. */
uint256 lusdToAcquire = accruedSLUSD * backingRatio / 1e18;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if in _calcAccruedSLUSD we first apply the time factor (* t / (t+u) or whatever) and then the backing ratio. That function could return both the value in sLUSD and LUSD, so we don’t divide by the backing ratio and then multiply again. I mean for better precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that sounds good to me

uint256 lusdToAcquire = accruedSLUSD * backingRatio / 1e18;
uint256 lusdToRefund = bond.lusdAmount - lusdToAcquire;

assert ((lusdToAcquire + lusdToRefund) <= bond.lusdAmount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t this redundant with the previous subtraction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a pretty obviously true assert - no real need for it.

function redeem(uint256 _sLUSDToRedeem) external {
_requireNonZeroAmount(_sLUSDToRedeem);

/* TODO: determine whether we should simply leave the fee in the acquired bucket, or add it to a permanent bucket.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we don’t have a good definition for fair price yet, it seems to me that leaving them in the acquired bucket would increase the redemption price, while adding them to the permanent bucket would increase the fair price.
So I would do the latter, to increase the premium. (If we decide so, I can do it when I implement the liquity-like redemption fee)
Thoughts @cvalkan ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think the fair price increases in both cases (according to the naive formula at least), but the premium would only increase if we add the fee to the permanent bucket.

I agree that we should do that since increasing the premium is more important for overall system health. This is a bit outdated, but you can also find some more thoughts here: #4.

* chicken-in value so that acquired LUSD always more than covers any rounding error in the share value.
*/
uint256 acquiredLUSDInYearn = _lusdInYearn > totalPendingLUSD ? _lusdInYearn - totalPendingLUSD : 0;
assert(acquiredLUSDInYearn >= 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acquiredLUSDInYearn is a uint, so it would revert previously otherwise, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that assert isn't adding much here

* TODO: Determine if this is the only situation whereby the delta can be negative. Potentially enforce some minimum
* chicken-in value so that acquired LUSD always more than covers any rounding error in the share value.
*/
uint256 acquiredLUSDInYearn = _lusdInYearn > totalPendingLUSD ? _lusdInYearn - totalPendingLUSD : 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you meant to use totalPendingLUSDCached here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! good catch (and saw you fixed in a commit)

if (yTokensToWithdrawFromCurveVault > 0) {yearnCurveVault.withdraw(yTokensToWithdrawFromCurveVault);} // obtain LUSD3CRV from Yearn

uint256 LUSD3CRVDelta = curvePool.balanceOf(address(this)) - LUSD3CRVBalanceBefore;
if (LUSD3CRVDelta > 0) {curvePool.remove_liquidity_one_coin(LUSD3CRVDelta, INDEX_OF_LUSD_TOKEN_IN_CURVE_POOL, 0);} // obtain LUSD from Curve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check the balance after this? I guess it may unbalance the pool.

Copy link
Contributor Author

@RickGriff RickGriff Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - it changes the Curve pool spot price (btw, you mentioned checking the balance, I assume you mean checking the Curve price?).

Currently, redemptions are taken proportionally from the SP and Curve. As long as redemptions are drawn from Curve in pure LUSD, they increase the Curve spot price. IIRC we touched on this briefly at one point, but we haven't revisited.

For spot_price < 1, this is beneficial and brings the price back to peg.

However, if spot_price >= 1 redemptions further increase the price above the peg - that's undesirable.

Some possible solutions:

  • When Curve price >= 1, disable redemptions or pull them purely from the Stability Pool (impacts the yield)
  • Redeem proportionally from SP and Curve, and make the Curve portion paid in both LUSD and 3CRV (or LUSD and USDC) in order to maintain the current Curve spot price

The latter seems smoothest from an economic perspective. And Curve allow us to very easily remove_liquidity in a ratio that maintains the current spot price. The only downside I can see is that our system ends up partly issuing 3CRV or USDC to redeemers, so it's not 100% pure LUSD. Though, chicken-outs are still in pure LUSD, which seems more important. @cvalkan, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer the latter solution, i.e. paying out both coins on redemption. If we were to disable redemptions above some price, there would be no hard guarantee on how fast the sLUSD supply can be redeemed, e.g. in case of a migration. Especially if a significant portion of the Curve pool ends up being contributed by the protocol.


import "../../lib/openzeppelin-contracts/contracts/interfaces/IERC20.sol";

interface ILUSDToken is IERC20 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No big deal, but here we only need the IERC20 interface, right?

address _sLUSDTokenAddress,
address _yearnRegistryAddress
)
public onlyOwner
Copy link
Contributor

@bingen bingen Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, do we need onlyOwner?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we don't need it in the constructor (as per your prev. commit)

Copy link
Contributor

@danielattilasimon danielattilasimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great already 💯 ! There are some parts to revisit later (already marked by TODOs 👍), and some edge cases to test in more detail (like lossy harvests), but I think the branch is ready to be merged already.

address _sLUSDTokenAddress,
address _yearnRegistryAddress
)
public onlyOwner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how does onlyOwner work on a constructor? I suppose it must be checked after the super-constructor (Ownable's) has already been executed, setting owner to msg.sender, otherwise it wouldn't work.

Either way, it seems redundant, as the owner will be the caller by definition.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also – this is just a minor nitpick – visibility modifiers on constructors are obsolete since 0.7:
https://docs.soliditylang.org/en/v0.7.0/070-breaking-changes.html#functions-and-events



// TODO: Decide between one-time infinite LUSD approval to Yearn and Curve (lower gas cost per user tx, less secure)
// or limited approval at each bonder action (higher gas cost per user tx, more secure)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, individual, finite approvals before each call seems like the safer bet. Otherwise, the 3rd party may upgrade their contracts, adding a function to sweep LUSD from our contract.

* TODO: replace with logic that calculates the LUSD quantity to shift based on a resulting Curve spot price that does not cross the
boundary price of 1. Requires mathematical formula for quantity based on price (i.e. rearrange Curve spot price formula)
*
* Simple alternative: make the outer shift function revert if the resulting LUSD spot price on Curve has crossed the boundary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer the simple alternative here. The math involved might be too costly for EVM anyway.

if (yTokensToWithdrawFromCurveVault > 0) {yearnCurveVault.withdraw(yTokensToWithdrawFromCurveVault);} // obtain LUSD3CRV from Yearn

uint256 LUSD3CRVDelta = curvePool.balanceOf(address(this)) - LUSD3CRVBalanceBefore;
if (LUSD3CRVDelta > 0) {curvePool.remove_liquidity_one_coin(LUSD3CRVDelta, INDEX_OF_LUSD_TOKEN_IN_CURVE_POOL, 0);} // obtain LUSD from Curve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer the latter solution, i.e. paying out both coins on redemption. If we were to disable redemptions above some price, there would be no hard guarantee on how fast the sLUSD supply can be redeemed, e.g. in case of a migration. Especially if a significant portion of the Curve pool ends up being contributed by the protocol.

* also when the sLUSD supply is fully redeemed.
*/
if (totalSLUSDSupply == 0 && totalAcquiredLUSD == 0) {return 1e18;}
if (totalSLUSDSupply == 0) {return MAX_UINT256;}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we run into this case in practice, i.e. that there's acquired LUSD but no sLUSD issued? Sounds like it shouldn't happen.

@RickGriff RickGriff merged commit 4838b74 into main Apr 13, 2022
bingen added a commit that referenced this pull request May 22, 2022
@danielattilasimon danielattilasimon deleted the lusd_chicken_bonds branch September 26, 2022 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants