Improvement/aura fed to base fed #9
Conversation
| function _addLiquidity(uint dolaAmount, uint maxSlippage) internal returns(uint){ | ||
| uint initialBal = BPT.balanceOf(address(this)); | ||
| uint BPTWanted = bptNeededForDola(dolaAmount); | ||
| uint minBptOut = BPTWanted - BPTWanted * maxSlippage / BPS; |
There was a problem hiding this comment.
This is sandwichable because calculations can be affected by a front-runner. Be sure to use private mempool or add in a function parameter which allows the fed chair to pass in an off-chain computed value.
Edit: Actually, i think it is not sandwichable since it is a stable pool and the function considers the assets pegged, right?
There was a problem hiding this comment.
It is sandwhichable up to the maxSlippage amount. We generally try to execute with flashbots rpc, but I can definitely see the idea in adding a "minBPT" or similar.
| function _addLiquidity(uint dolaAmount, uint maxSlippage) internal returns(uint){ | ||
| uint initialBal = BPT.balanceOf(address(this)); | ||
| uint BPTWanted = bptNeededForDola(dolaAmount); | ||
| uint minBptOut = BPTWanted - BPTWanted * maxSlippage / BPS; |
There was a problem hiding this comment.
Might consider adding parentheses for order of operations clarity.
| _; | ||
| } | ||
|
|
||
| function setMaxLossExpansionBps(uint newMaxLossExpansionBps) onlyGov external { |
There was a problem hiding this comment.
Updating max loss on Expansions and TakeProfit are gov only, but Contractions can be guardian as well. Is the difference due to one being more sensitive than the other?
There was a problem hiding this comment.
Yeah, idea is that an emergency contraction may need to happen in short order, due to a hack of an underlying protocol. We saw that play out during the Euler hack, where fast contractions were instrumental in avoiding as much bad debt as possible.
|
|
||
| /** | ||
| * @notice Migrates both claims and debt from another fed to this one | ||
| * @dev Must call the migraTo function of the target fed |
| @@ -128,14 +143,36 @@ abstract contract BaseFed { | |||
| function takeProfit(uint flag) external virtual; | |||
There was a problem hiding this comment.
Small suggestion. Can try a pattern like this which enforces events are always implemented without relying on developer to remember.
function takeProfit(uint flag) external {
(address token, uint amount) = _takeProfit(flag);
Profit(token, amount);
}
function _takeProfit() external virtual returns(address token, uint amount);
| uint bptNeeded = bptNeededForDola(dolaAmount); | ||
| function _removeLiquidity(uint dolaAmount, uint maxSlippage) internal returns(uint){ | ||
| uint initialBal = _DOLA.balanceOf(address(this)); | ||
| uint BPTNeeded = bptNeededForDola(dolaAmount); | ||
| uint minDolaOut = dolaAmount - dolaAmount * maxSlippage / BPS; |
There was a problem hiding this comment.
Consider parentheses for order of operations clarity
| uint bptNeeded = bptNeededForDola(amountDola); | ||
| require(bptNeeded <= bptSupply(), "Not enough BPT tokens"); | ||
| require(bptNeeded <= claimsSupply(), "Not enough BPT tokens"); |
There was a problem hiding this comment.
could save some gas by using claimsBefore value here instead of recalculating claimsSupply().
| * @param amountDola The amount of dola tokens to withdraw. Note that more tokens may | ||
| * be withdrawn than requested, as price is calculated by debts to strategies, but strategies | ||
| * may have outperformed price of dola token. | ||
| * @return claimsUsed The amoutn of balancer pools tokens spent on withdrawing. |
| require(totalClaims >= claimsToMigrate, "NOT ENOUGH CLAIMS"); | ||
| uint claimsToUnstake = claimsToMigrate - BPT.balanceOf(address(this)); | ||
| require(dolaBptRewardPool.withdrawAndUnwrap(claimsToUnstake, false), "AURA WITHDRAW FAILED"); | ||
| uint debtToMigrate = debt * claimsToMigrate / totalClaims; |
There was a problem hiding this comment.
is it possible that there is ever debt > 0, and also totalClaims == 0?
If so this would revert when dividing by zero and wouldn't allow us to move that debt from one Fed to another.
There was a problem hiding this comment.
It's possible but at that point there would be no point in migrating, as the debt is purely bad debt and it has no claims to back it up. Becomes the job of the treasury to pay off the bad debt accumulated by the Fed.
| swapExactIn(address(bpt), address(dola), bptBal, minDolaOut); | ||
| return dola.balanceOf(address(this)); | ||
| swapExactIn(address(BPT), address(_DOLA), BPTBal, minDolaOut); | ||
| return _DOLA.balanceOf(address(this)); |
There was a problem hiding this comment.
info:
Inconsistent return value behavior between _removeLiquidity() and _removeAllLiquidity().
The latter does not account for any pre-exiting DOLA in the contract.
There was a problem hiding this comment.
It's working as intended. When calling the _removeLiquidity function, the intention for the Fed should be to remove a certain amount of DOLA from the active market, where _removeAllLiquidity is meant to wind down the Fed operation.
I suppose we could return the additional DOLA, but that's ultimately immaterial, as aslong the DOLA is outside of the market, then it's not influencing prices, and we can either take it as profit or burn it when winding down the Fed. In the general, DOLA existing within the mainnet AuraFed would be an anomaly.
Reimplements the AuraFed as the LossyFed abstract class, adding new functionality like:
And generally better assurances of accounting and access control logic being correctly implemented.