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

Revert on zero amounts #636

Merged
merged 10 commits into from
Mar 1, 2023
2 changes: 1 addition & 1 deletion docs/Functions.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
reverts on:
- deposits locked RemoveDepositLockedByAuctionDebt()
- LenderActions.moveQuoteToken():
- same index MoveToSamePrice()
- same index MoveToSameIndex()
- dust amount DustAmountNotExceeded()
- invalid index InvalidIndex()

Expand Down
6 changes: 6 additions & 0 deletions src/base/Pool.sol
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,12 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
);
}

/// @inheritdoc IPoolLenderActions
function updateInterest() external override nonReentrant {
PoolState memory poolState = _accruePoolInterest();
_updateInterestState(poolState, _lup(poolState.debt));
}

/*****************************/
/*** Liquidation Functions ***/
/*****************************/
Expand Down
14 changes: 12 additions & 2 deletions src/interfaces/pool/commons/IPoolErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,21 @@ interface IPoolErrors {
*/
error InsufficientLiquidity();

/**
* @notice When settling pool debt the number of buckets to use should be greater than 0.
*/
error InvalidBucketDepth();

/**
* @notice When transferring LPs between indices, the new index must be a valid index.
*/
error InvalidIndex();

/**
* @notice The amount used for performed action should be greater than 0.
*/
error InvalidAmount();

/**
* @notice Borrower is attempting to borrow more quote token than is available before the supplied limitIndex.
*/
Expand All @@ -129,9 +139,9 @@ interface IPoolErrors {
error LUPGreaterThanTP();

/**
* @notice FromIndex_ and toIndex_ arguments to move are the same.
* @notice From index and to index arguments to move are the same.
*/
error MoveToSamePrice();
error MoveToSameIndex();

/**
* @notice Owner of the LPs must have approved the new owner prior to transfer.
Expand Down
5 changes: 5 additions & 0 deletions src/interfaces/pool/commons/IPoolLenderActions.sol
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,9 @@ interface IPoolLenderActions {
address newOwner,
uint256[] calldata indexes
) external;

/**
* @notice Called by lenders to update pool interest rate (can be updated only once in a 12 hours period of time).
*/
function updateInterest() external;
}
12 changes: 11 additions & 1 deletion src/libraries/external/Auctions.sol
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ library Auctions {
error CollateralRoundingNeededButNotPossible();
error InsufficientLiquidity();
error InsufficientCollateral();
error InvalidBucketDepth();
error InvalidAmount();
error NoAuction();
error NoReserves();
error NoReservesAuction();
Expand Down Expand Up @@ -205,6 +207,9 @@ library Auctions {
uint256 collateralSettled_,
uint256 t0DebtSettled_
) {
// revert if no bucket to settle
if (params_.bucketDepth == 0 ) revert InvalidBucketDepth();
mattcushman marked this conversation as resolved.
Show resolved Hide resolved

uint256 kickTime = auctions_.liquidations[params_.borrower].kickTime;
if (kickTime == 0) revert NoAuction();

Expand Down Expand Up @@ -559,10 +564,12 @@ library Auctions {
uint256 collateral_,
uint256 collateralScale_
) external returns (TakeResult memory result_) {
// revert if no amount to take
if (collateral_ == 0) revert InvalidAmount();

Borrower memory borrower = loans_.borrowers[borrowerAddress_];

if (
(collateral_ == 0) || // revert if amount to take is 0
(poolState_.poolType == uint8(PoolType.ERC721) && borrower.collateral < 1e18) || // revert in case of NFT take when there isn't a full token to be taken
(poolState_.poolType == uint8(PoolType.ERC20) && borrower.collateral == 0) // revert in case of ERC20 take when no collateral to be taken
) {
Expand Down Expand Up @@ -693,6 +700,9 @@ library Auctions {
ReserveAuctionState storage reserveAuction_,
uint256 maxAmount_
) external returns (uint256 amount_, uint256 ajnaRequired_) {
// revert if no amount to be taken
if (maxAmount_ == 0) revert InvalidAmount();

uint256 kicked = reserveAuction_.kicked;

if (kicked != 0 && block.timestamp - kicked <= 72 hours) {
Expand Down
33 changes: 15 additions & 18 deletions src/libraries/external/BorrowerActions.sol
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ library BorrowerActions {
error BorrowerNotSender();
error BorrowerUnderCollateralized();
error InsufficientCollateral();
error InvalidAmount();
error LimitIndexExceeded();
error NoDebt();

Expand Down Expand Up @@ -117,12 +118,15 @@ library BorrowerActions {
) external returns (
DrawDebtResult memory result_
) {
Borrower memory borrower = loans_.borrowers[borrowerAddress_];

DrawDebtLocalVars memory vars;
vars.pledge = collateralToPledge_ != 0;
vars.borrow = amountToBorrow_ != 0;

// revert if no amount to pledge or borrow
if (!vars.pledge && !vars.borrow) revert InvalidAmount();

Borrower memory borrower = loans_.borrowers[borrowerAddress_];

vars.pledge = collateralToPledge_ != 0;
vars.borrow = amountToBorrow_ != 0 || limitIndex_ != 0; // enable an intentional 0 borrow loan call to update borrower's loan state
vars.borrowerDebt = Maths.wmul(borrower.t0Debt, poolState_.inflator);
vars.inAuction = _inAuction(auctions_, borrowerAddress_);

Expand Down Expand Up @@ -219,11 +223,6 @@ library BorrowerActions {
vars.stampT0Np = true;
}

// calculate LUP if it wasn't calculated previously
if (!vars.pledge && !vars.borrow) {
result_.newLup = _lup(deposits_, result_.poolDebt);
}

// update loan state
Loans.update(
loans_,
Expand Down Expand Up @@ -277,12 +276,15 @@ library BorrowerActions {
) external returns (
RepayDebtResult memory result_
) {
Borrower memory borrower = loans_.borrowers[borrowerAddress_];

RepayDebtLocalVars memory vars;
vars.repay = maxQuoteTokenAmountToRepay_ != 0;
vars.pull = collateralAmountToPull_ != 0;

// revert if no amount to pledge or borrow
if (!vars.repay && !vars.pull) revert InvalidAmount();

Borrower memory borrower = loans_.borrowers[borrowerAddress_];

vars.repay = maxQuoteTokenAmountToRepay_ != 0;
vars.pull = collateralAmountToPull_ != 0;
vars.borrowerDebt = Maths.wmul(borrower.t0Debt, poolState_.inflator);
vars.inAuction = _inAuction(auctions_, borrowerAddress_);

Expand Down Expand Up @@ -379,11 +381,6 @@ library BorrowerActions {
result_.poolCollateral -= collateralAmountToPull_;
}

// calculate LUP if it wasn't calculated previously
if (!vars.repay && !vars.pull) {
result_.newLup = _lup(deposits_, result_.poolDebt);
}

// update loan state
Loans.update(
loans_,
Expand Down
24 changes: 21 additions & 3 deletions src/libraries/external/LenderActions.sol
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,13 @@ library LenderActions {
error DustAmountNotExceeded();
error NoAllowance();
error InvalidIndex();
error InvalidAmount();
error LUPBelowHTP();
error NoClaim();
error InsufficientLPs();
error InsufficientLiquidity();
error InsufficientCollateral();
error MoveToSamePrice();
error MoveToSameIndex();
error TransferorNotApproved();
error TransferToSameOwner();

Expand All @@ -110,6 +111,9 @@ library LenderActions {
uint256 collateralAmountToAdd_,
uint256 index_
) external returns (uint256 bucketLPs_) {
// revert if no amount to be added
if (collateralAmountToAdd_ == 0) revert InvalidAmount();
// revert if adding at invalid index
if (index_ == 0 || index_ > MAX_FENWICK_INDEX) revert InvalidIndex();

uint256 bucketDeposit = Deposits.valueAt(deposits_, index_);
Expand Down Expand Up @@ -143,6 +147,9 @@ library LenderActions {
PoolState calldata poolState_,
AddQuoteParams calldata params_
) external returns (uint256 bucketLPs_, uint256 lup_) {
// revert if no amount to be added
if (params_.amount == 0) revert InvalidAmount();
// revert if adding to an invalid index
if (params_.index == 0 || params_.index > MAX_FENWICK_INDEX) revert InvalidIndex();

Bucket storage bucket = buckets_[params_.index];
Expand Down Expand Up @@ -196,7 +203,7 @@ library LenderActions {
* - decrement bucket.lps accumulator for from bucket
* - increment bucket.lps accumulator for to bucket
* @dev reverts on:
* - same index MoveToSamePrice()
* - same index MoveToSameIndex()
* - dust amount DustAmountNotExceeded()
* - invalid index InvalidIndex()
* @dev emit events:
Expand All @@ -209,8 +216,10 @@ library LenderActions {
PoolState calldata poolState_,
MoveQuoteParams calldata params_
) external returns (uint256 fromBucketRedeemedLPs_, uint256 toBucketLPs_, uint256 movedAmount_, uint256 lup_) {
if (params_.maxAmountToMove == 0)
revert InvalidAmount();
if (params_.fromIndex == params_.toIndex)
revert MoveToSamePrice();
revert MoveToSameIndex();
if (params_.maxAmountToMove != 0 && params_.maxAmountToMove < poolState_.quoteDustLimit)
revert DustAmountNotExceeded();
if (params_.toIndex == 0 || params_.toIndex > MAX_FENWICK_INDEX)
Expand Down Expand Up @@ -344,6 +353,9 @@ library LenderActions {
PoolState calldata poolState_,
RemoveQuoteParams calldata params_
) external returns (uint256 removedAmount_, uint256 redeemedLPs_, uint256 lup_) {
// revert if no amount to be removed
if (params_.maxAmount == 0) revert InvalidAmount();

Bucket storage bucket = buckets_[params_.index];
Lender storage lender = bucket.lenders[msg.sender];

Expand Down Expand Up @@ -415,6 +427,9 @@ library LenderActions {
uint256 amount_,
uint256 index_
) external returns (uint256 lpAmount_) {
// revert if no amount to be removed
if (amount_ == 0) revert InvalidAmount();

Bucket storage bucket = buckets_[index_];

uint256 bucketCollateral = bucket.collateral;
Expand Down Expand Up @@ -480,6 +495,9 @@ library LenderActions {
uint256 maxAmount_,
uint256 index_
) external returns (uint256, uint256) {
// revert if no amount to remove
if (maxAmount_ == 0) revert InvalidAmount();

return _removeMaxCollateral(
buckets_,
deposits_,
Expand Down
22 changes: 4 additions & 18 deletions tests/forge/ERC20Pool/ERC20DSTestPlus.sol
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,10 @@ abstract contract ERC20DSTestPlus is DSTestPlus, IERC20PoolEvents {
// mint quote tokens to borrower address equivalent to the current debt
deal(_pool.quoteTokenAddress(), borrower, currentDebt);

// repay current debt and pull all collateral
_repayDebtNoLupCheck(borrower, borrower, tokenDebt, currentDebt, borrowerCollateral);
// repay current debt and pull all collateral if any
if (tokenDebt != 0 || borrowerCollateral != 0) {
_repayDebtNoLupCheck(borrower, borrower, tokenDebt, currentDebt, borrowerCollateral);
}

// check borrower state after repay of loan and pull collateral
(borrowerT0debt, borrowerCollateral, ) = _pool.borrowerInfo(borrower);
Expand Down Expand Up @@ -237,22 +239,6 @@ abstract contract ERC20DSTestPlus is DSTestPlus, IERC20PoolEvents {
borrowers.add(from);
}

function _borrowZeroAmount(
address from,
uint256 amount,
uint256 indexLimit,
uint256 newLup
) internal {
changePrank(from);
vm.expectEmit(true, true, false, true);
emit DrawDebt(from, amount, 0, newLup);

ERC20Pool(address(_pool)).drawDebt(from, amount, indexLimit, 0);

// Add for tearDown
borrowers.add(from);
}

function _drawDebt(
address from,
address borrower,
Expand Down
16 changes: 5 additions & 11 deletions tests/forge/ERC20Pool/ERC20PoolBorrow.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -432,12 +432,7 @@ contract ERC20PoolBorrowTest is ERC20HelperContract {

skip(10 days);

_borrowZeroAmount({
from: _borrower,
amount: 0,
indexLimit: 3_000,
newLup: 2_981.007422784467321543 * 1e18
});
_updateInterest();

expectedDebt = 21_157.152643010853304038 * 1e18;

Expand All @@ -462,15 +457,14 @@ contract ERC20PoolBorrowTest is ERC20HelperContract {
borrower: _borrower,
borrowerDebt: expectedDebt,
borrowerCollateral: 50 * 1e18,
borrowert0Np: 448.381722115384615591 * 1e18,
borrowert0Np: 445.838278846153846359 * 1e18,
Copy link
Contributor

Choose a reason for hiding this comment

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

Conceivably, borrowing zero (old line 435) was a way to update the borrower's neutral price. What's the new mechanism for doing so? Drawing or repaying 1 wei or whatever the dust limit is?

Copy link
Contributor Author

@grandizzy grandizzy Feb 28, 2023

Choose a reason for hiding this comment

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

Must admit I don't understand the mechanism of updating NP by borrowing 0, can you explain what's the scenario that is used for? Maybe if we need such out of context of draw/repay debt we should expose an updateNP function for borrowers?

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of this is rudimentary. A borrower's neutral price is a function of the MOMP and LUP, which change over time. Before a borrower is liquidated, I thought the borrower had the right to restamp their neutral price if conditions were favorable to do so. That's what BorrowerActions old line 125 was permitting, with the restamp happening on old line 219.

Off topic: I do not understand why the borrower needs a neutral price stamp at all. Seems it would be easier to stamp that on the liquidation upon kick and calculate it on-the-fly until then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! @mattcushman can you please share your thoughts on this matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for stamping the neutral price on the loan at borrowing is to provide some certainty to the borrower as to at what price they can expect to be liquidated. If we made it entirely dynamic, it would invite kickets to try to lower the dynamic measure of price in such a way to make kicking the loans profitable. We struggled to find a good dynamic measure, but this made the most sense over the lifetime of the loan.

And yes, the borrower should be able to restamp their NP as long as they are fully collateralized. THey can always trivially do so by removing 1 unit of debt or collateral.

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, can add such function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattcushman should we allow other parties but borrower to restamp the loan? I think we should as we're currently allowing this as draw / repay debt can be invoked by different actors but want to get your confirmation on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added with e07f6bd please review

Copy link
Contributor Author

@grandizzy grandizzy Feb 28, 2023

Choose a reason for hiding this comment

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

per our discussion 08b8943 removes the ability of 3rd parties to restamp Neutral Price of loan

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

borrowerCollateralization: 7.044916376706357984 * 1e18
});
_assertLenderInterest(liquidityAdded, 119.836959946754650000 * 1e18);

skip(10 days);

// call drawDebt to restamp the loan's neutral price
IERC20Pool(address(_pool)).drawDebt(_borrower, 0, 0, 0);
_updateInterest();

expectedDebt = 21_199.628356897284442294 * 1e18;

Expand All @@ -495,7 +489,7 @@ contract ERC20PoolBorrowTest is ERC20HelperContract {
borrower: _borrower,
borrowerDebt: expectedDebt,
borrowerCollateral: 50 * 1e18,
borrowert0Np: 448.381722115384615591 * 1e18,
borrowert0Np: 445.838278846153846359 * 1e18,
borrowerCollateralization: 7.030801136225104190 * 1e18
});
_assertLenderInterest(liquidityAdded, 157.005764521268350000 * 1e18);
Expand Down Expand Up @@ -526,7 +520,7 @@ contract ERC20PoolBorrowTest is ERC20HelperContract {
borrower: _borrower,
borrowerDebt: expectedDebt,
borrowerCollateral: 50 * 1e18,
borrowert0Np: 448.381722115384615591 * 1e18,
borrowert0Np: 445.838278846153846359 * 1e18,
borrowerCollateralization: 7.015307034516347067 * 1e18
});
}
Expand Down
Loading