feat: Add multicollateral registry and weakest collateral redemption #127
Conversation
bingen
force-pushed
the
multicollateral_1_registry
branch
3 times, most recently
from
2843b68
to
bd3224b
Compare
bingen
force-pushed
the
multicollateral_1_registry
branch
from
bd3224b
to
4ac8dc3
Compare
| mapping(address => bool) troveManagerAddresses; | ||
| mapping(address => bool) stabilityPoolAddresses; | ||
| mapping(address => bool) borrowerOperationsAddresses; | ||
| mapping(address => bool) activePoolAddresses; |
There was a problem hiding this comment.
address public immutable troveManagerAddress0;
address public immutable troveManagerAddress1;
// ...
address public immutable troveManagerAddress7;
address public immutable stabilityPoolAddress0;
address public immutable stabilityPoolAddress1;
// ...
address public immutable stabilityPoolAddress7;
// ...?
There was a problem hiding this comment.
When we get around to that, maybe we could put that (along with all the _requireXXXs) in a base contract to keep that pollution tucked away? 😄
There was a problem hiding this comment.
Yes, the idea was to do something like your suggestion above (and like CollateralRegistry), I’ll change it when the rest of the PR is done.
Address PR #127 comments.
Convert it to a wrapper of assertApproxEqAbs
contracts/src/CollateralRegistry.sol
Outdated
|
|
||
| // Decay the baseRate due to time passed, and then increase it according to the size of this redemption. | ||
| // Use the saved total Bold supply value, from before it was reduced by the redemption. | ||
| // TODO: what if the final redeemed amount is less than the requested amount? |
There was a problem hiding this comment.
Yeah, as it stands, a griefer could just flashloan a huge wad of BOLD (in addition to a relatively small amount of their own funds), set _maxIterations to 1, create minimal non-zombies at the start of each SortedTroves list, and cause a huge spike in base rate by redeeming that wad of BOLD. Economically, this doesn't seem to make sense, but the cost of it is way too low (< the capital required to open the minimal Troves, as the overcollateral part and the ETH not eaten up by redemption fees is recovered).
We need to move the calculation of the fee and payout of ETH out of TroveManager::redeemCollateral() and perform the redemption in 2 stages:
- redeem from each branch
- (in the registry, tally the amount redeemed, calculate fee)
- payout ETH from each branch, deducting the fee
There was a problem hiding this comment.
Actually, I guess a whale could even use this to make redemptions against their huge Trove unprofitable, while paying minimal interest.
There was a problem hiding this comment.
Upon discussing this with @cvalkan on Slack, my proposal is not feasible, because we want to keep the redemption fee inside the redeemed Troves. So unless we want to go back and iterate over the Troves again (let's not do that), we have to commit to a fee percentage at the start.
To avoid attacks like the one outlined above, we should make sure that the post-redemption spike of the base rate is based on the actual redeemed amount instead of the attempted amount though.
contracts/src/CollateralRegistry.sol
Outdated
| if (redeemAmount > 0) { | ||
| ITroveManager troveManager = getTroveManager(index); | ||
| uint256 feePercentage = troveManager.redeemCollateral(msg.sender, redeemAmount, _maxIterations); | ||
| totalFeePercentage += feePercentage * redeemAmount; |
There was a problem hiding this comment.
Just reading this line as-is I initially thought it was a mistake, but the final calculation is correct.
Perhaps we could use an intermediate variable here, since (as you show below), the totalFeePercentage must be calculated using the totalRedeemedAmount.
(This intermediate var could be feeWeightedRedeemAmount or something)
There was a problem hiding this comment.
Hm, this should be now changed, after discussion on Slack. The fee is now common, based in total Bold supply. Can you refresh in your browser?
| const dumbContract = await GasPool.new(); | ||
| const params = Array(numberOfAddresses).fill(dumbContract.address); | ||
|
|
||
| // Attempt call from alice | ||
| await th.assertRevert(contract.setAddresses(...params, { from: alice })); | ||
| await th.assertRevert(contract[method](...params, { from: alice })); |
There was a problem hiding this comment.
What's the reason for this change?
There was a problem hiding this comment.
That now I’m using it also for other methods, like setCollateralRegistry or setBranchAddresses. I’ll change the function name so it’s clearer.
contracts/src/CollateralRegistry.sol
Outdated
| } | ||
|
|
||
| // Updates the `baseRate` state with math from `_getUpdatedBaseRateFromRedemption` | ||
| function _updateBaseRateAndGetRedemptionRate(IBoldToken _boldToken, uint256 _boldAmount, uint256 _totalBoldSupplyAtStart) |
There was a problem hiding this comment.
_boldToken is unused now.
There was a problem hiding this comment.
Nice, thanks! It would be nice to be able to see warnings only from current changes.
bingen
force-pushed
the
multicollateral_1_registry
branch
from
42f2bed
to
6036ad7
Compare
Closes #90