Take underflows when full pool debt repaid (#551)

Take underflows when full pool debt repaid Scenario is exposed in testTakeAuctionRepaidAmountGreaterThanPoolDebt test: - when auction debt is fully repaid by take action then accrued pool debt value is less than repaid amount with 1 unit of WAD. When the repaid amount is subtracted from pool debt value (normally leaving no debt in pool) the operation underflows. - this happens because repaid debt is calculated using t0 value (t0 repaid debt * inflator) and then subtracted from already calculated pool debt - fix is to calculate pool debt as (t0 pool debt - t0 repaid debt) * inflator, hence preventing rounding issues The scope of this PR is extended to uniformize the t0 / accrued debt values calculation across all contracts by using pattern below: - when values relative to t0 are changed then t0 is updated first and then transformed in actual value (by multiplying it with inflator value) - all accumulations of pool or borrower debt are done on spot and not deferred to Pool contracts (which only save states)
ajna-finance · Jan 31, 2023 · 7127dff · 7127dff
1 parent fa3a150
commit 7127dff
Show file tree

Hide file tree

Showing 13 changed files with 359 additions and 234 deletions.
diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md
@@ -0,0 +1,37 @@
+
+<!---
+No need to add special tag
+src/ & non src/ changes you need the following (that apply):
+-->
+# Description of change
+## High level
+* <DESCRIP_OF_CHANGE>
+ * <DESCRIP_OF_SUBCHANGE>
+
+<!---
+Add the `Status: Needs Auditor Approval` tags
+CHANGES IN /SRC DIR:
+- renaming (not retyping or resizing) of variables & methods
+- reordering and moving of functions in files
+- lite moving of functions accross files
+- comments
+
+src/ changes you need the following (that apply):
+-->
+
+# Description of bug or vulnerability and solution
+* <PARAGRAPH_EXP_OF_VULN_BUG>
+* <PARAGRAPH_EXP_OF_HOW_CHANGE_SOLVES_VULN_OR_BUG>
+
+# Contract size
+## Pre Change
+<PASTE_OUTPUT_HERE>
+## Post Change
+<PASTE_OUTPUT_HERE>
+
+# Gas usage
+## Pre Change
+<PASTE_OUTPUT_HERE>
+## Post Change
+<PASTE_OUTPUT_HERE>
+
diff --git a/src/ERC20Pool.sol b/src/ERC20Pool.sol
@@ -165,7 +165,7 @@ contract ERC20Pool is FlashloanablePool, IERC20Pool {
 
  if (amountToBorrow_ != 0) {
  // update pool balances state
- poolBalances.t0Debt += result.t0DebtChange;
+ poolBalances.t0Debt = result.t0PoolDebt;
 
  // move borrowed amount from pool to sender
  _transferQuoteToken(msg.sender, amountToBorrow_);
@@ -212,7 +212,7 @@ contract ERC20Pool is FlashloanablePool, IERC20Pool {
 
  if (result.quoteTokenToRepay != 0) {
  // update pool balances state
- poolBalances.t0Debt -= result.t0RepaidDebt;
+ poolBalances.t0Debt = result.t0PoolDebt;
  if (result.t0DebtInAuctionChange != 0) {
  poolBalances.t0DebtInAuction -= result.t0DebtInAuctionChange;
  }
@@ -427,18 +427,11 @@ contract ERC20Pool is FlashloanablePool, IERC20Pool {
  result.quoteTokenAmount = _roundUpToScale(result.quoteTokenAmount, _getArgUint256(QUOTE_SCALE));
 
  // update pool balances state
- uint256 t0PoolDebt = poolBalances.t0Debt;
  uint256 t0DebtInAuction = poolBalances.t0DebtInAuction;
-
- if (result.t0DebtPenalty != 0) {
- t0PoolDebt += result.t0DebtPenalty;
- t0DebtInAuction += result.t0DebtPenalty;
- }
-
- t0PoolDebt -= result.t0RepayAmount;
+ t0DebtInAuction += result.t0DebtPenalty;
  t0DebtInAuction -= result.t0DebtInAuctionChange;
 
- poolBalances.t0Debt = t0PoolDebt;
+ poolBalances.t0Debt = result.t0PoolDebt;
  poolBalances.t0DebtInAuction = t0DebtInAuction;
  poolBalances.pledgedCollateral -= result.collateralAmount;
 
@@ -488,18 +481,11 @@ contract ERC20Pool is FlashloanablePool, IERC20Pool {
  );
 
  // update pool balances state
- uint256 t0PoolDebt = poolBalances.t0Debt;
  uint256 t0DebtInAuction = poolBalances.t0DebtInAuction;
-
- if (result.t0DebtPenalty != 0) {
- t0PoolDebt += result.t0DebtPenalty;
- t0DebtInAuction += result.t0DebtPenalty;
- }
-
- t0PoolDebt -= result.t0RepayAmount;
+ t0DebtInAuction += result.t0DebtPenalty;
  t0DebtInAuction -= result.t0DebtInAuctionChange;
 
- poolBalances.t0Debt = t0PoolDebt;
+ poolBalances.t0Debt = result.t0PoolDebt;
  poolBalances.t0DebtInAuction = t0DebtInAuction;
  poolBalances.pledgedCollateral -= result.collateralAmount;
 

diff --git a/src/ERC721Pool.sol b/src/ERC721Pool.sol
@@ -174,7 +174,7 @@ contract ERC721Pool is FlashloanablePool, IERC721Pool {
  // move borrowed amount from pool to sender
  if (amountToBorrow_ != 0) {
  // update pool balances state
- poolBalances.t0Debt += result.t0DebtChange;
+ poolBalances.t0Debt = result.t0PoolDebt;
 
  // move borrowed amount from pool to sender
  _transferQuoteToken(msg.sender, amountToBorrow_);
@@ -220,7 +220,7 @@ contract ERC721Pool is FlashloanablePool, IERC721Pool {
 
  if (result.quoteTokenToRepay != 0) {
  // update pool balances state
- poolBalances.t0Debt -= result.t0RepaidDebt;
+ poolBalances.t0Debt = result.t0PoolDebt;
  if (result.t0DebtInAuctionChange != 0) {
  poolBalances.t0DebtInAuction -= result.t0DebtInAuctionChange;
  }
@@ -422,18 +422,11 @@ contract ERC721Pool is FlashloanablePool, IERC721Pool {
  );
 
  // update pool balances state
- uint256 t0PoolDebt = poolBalances.t0Debt;
  uint256 t0DebtInAuction = poolBalances.t0DebtInAuction;
-
- if (result.t0DebtPenalty != 0) {
- t0PoolDebt += result.t0DebtPenalty;
- t0DebtInAuction += result.t0DebtPenalty;
- }
-
- t0PoolDebt -= result.t0RepayAmount;
+ t0DebtInAuction += result.t0DebtPenalty;
  t0DebtInAuction -= result.t0DebtInAuctionChange;
 
- poolBalances.t0Debt = t0PoolDebt;
+ poolBalances.t0Debt = result.t0PoolDebt;
  poolBalances.t0DebtInAuction = t0DebtInAuction;
  poolBalances.pledgedCollateral -= result.collateralAmount;
 
@@ -494,18 +487,11 @@ contract ERC721Pool is FlashloanablePool, IERC721Pool {
  );
 
  // update pool balances state
- uint256 t0PoolDebt = poolBalances.t0Debt;
  uint256 t0DebtInAuction = poolBalances.t0DebtInAuction;
-
- if (result.t0DebtPenalty != 0) {
- t0PoolDebt += result.t0DebtPenalty;
- t0DebtInAuction += result.t0DebtPenalty;
- }
-
- t0PoolDebt -= result.t0RepayAmount;
+ t0DebtInAuction += result.t0DebtPenalty;
  t0DebtInAuction -= result.t0DebtInAuctionChange;
 
- poolBalances.t0Debt = t0PoolDebt;
+ poolBalances.t0Debt = result.t0PoolDebt;
  poolBalances.t0DebtInAuction = t0DebtInAuction;
  poolBalances.pledgedCollateral -= result.collateralAmount;
 

diff --git a/src/base/Pool.sol b/src/base/Pool.sol
@@ -273,11 +273,11 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  );
 
  // update pool balances state
+ poolBalances.t0Debt = result.t0PoolDebt;
  poolBalances.t0DebtInAuction += result.t0KickedDebt;
- poolBalances.t0Debt += result.t0KickPenalty;
 
  // update pool interest rate state
- poolState.debt += result.kickPenalty;
+ poolState.debt = Maths.wmul(result.t0PoolDebt, poolState.inflator);
  _updateInterestState(poolState, result.lup);
 
  if(result.amountToCoverBond != 0) _transferQuoteTokenFrom(msg.sender, result.amountToCoverBond);
@@ -304,11 +304,11 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  );
 
  // update pool balances state
- poolBalances.t0Debt += result.t0KickPenalty;
+ poolBalances.t0Debt = result.t0PoolDebt;
  poolBalances.t0DebtInAuction += result.t0KickedDebt;
 
  // update pool interest rate state
- poolState.debt += result.kickPenalty;
+ poolState.debt = Maths.wmul(result.t0PoolDebt, poolState.inflator);
  _updateInterestState(poolState, result.lup);
 
  // transfer from kicker to pool the difference to cover bond
@@ -356,7 +356,7 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  reserveAuction,
  StartReserveAuctionParams({
  poolSize: Deposits.treeSum(deposits),
- poolDebt:  poolBalances.t0Debt,
+ t0PoolDebt: poolBalances.t0Debt,
  poolBalance: _getPoolQuoteTokenBalance(),
  inflator: inflatorState.inflator
  })
@@ -424,20 +424,17 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  * @return poolState_ Struct containing pool details.
  */
  function _accruePoolInterest() internal returns (PoolState memory poolState_) {
- // retrieve t0Debt amount from poolBalances struct
- uint256 t0Debt = poolBalances.t0Debt;
-
- // initialize fields of poolState_ struct with initial values
+ poolState_.t0Debt = poolBalances.t0Debt;
  poolState_.collateral = poolBalances.pledgedCollateral;
  poolState_.inflator = inflatorState.inflator;
  poolState_.rate = interestState.interestRate;
  poolState_.poolType = _getArgUint8(POOL_TYPE);
  poolState_.quoteDustLimit = _getArgUint256(QUOTE_SCALE);
 
  // check if t0Debt is not equal to 0, indicating that there is debt to be tracked for the pool
- if (t0Debt != 0) {
+ if (poolState_.t0Debt != 0) {
  // Calculate prior pool debt
- poolState_.debt = Maths.wmul(t0Debt, poolState_.inflator);
+ poolState_.debt = Maths.wmul(poolState_.t0Debt, poolState_.inflator);
 
  // calculate elapsed time since inflator was last updated
  uint256 elapsed = block.timestamp - inflatorState.inflatorUpdate;
@@ -455,7 +452,7 @@ abstract contract Pool is Clone, ReentrancyGuard, Multicall, IPool {
  );
  poolState_.inflator = newInflator;
  // After debt owed to lenders has accrued, calculate current debt owed by borrowers
- poolState_.debt = Maths.wmul(t0Debt, poolState_.inflator);
+ poolState_.debt = Maths.wmul(poolState_.t0Debt, poolState_.inflator);
 
  // update total interest earned accumulator with the newly accrued interest
  reserveAuction.totalInterestEarned += newInterest;

diff --git a/src/interfaces/pool/commons/IPoolInternals.sol b/src/interfaces/pool/commons/IPoolInternals.sol
@@ -11,22 +11,21 @@ pragma solidity 0.8.14;
 /*****************************/
 
 struct BucketTakeResult {
- uint256 collateralAmount;
- uint256 t0RepayAmount;
- uint256 t0DebtPenalty;
- uint256 remainingCollateral;
- uint256 poolDebt;
- uint256 newLup;
- uint256 t0DebtInAuctionChange;
- bool settledAuction;
+ uint256 collateralAmount; // [WAD] amount of collateral taken
+ uint256 t0DebtPenalty; // [WAD] t0 penalty applied on first take
+ uint256 remainingCollateral; // [WAD] amount of borrower collateral remaining after take
+ uint256 poolDebt; // [WAD] current pool debt
+ uint256 t0PoolDebt; // [WAD] t0 pool debt
+ uint256 newLup; // [WAD] current lup
+ uint256 t0DebtInAuctionChange; // [WAD] the amount of t0 debt recovered by take action
+ bool settledAuction; // true if auction is settled by take action
 }
 
 struct KickResult {
- uint256 amountToCoverBond; // amount of bond that needs to be covered
- uint256 kickPenalty; // kick penalty
- uint256 t0KickPenalty; // t0 kick penalty
- uint256 t0KickedDebt; // new t0 debt after kick
- uint256 lup; // current lup
+ uint256 amountToCoverBond; // [WAD] amount of bond that needs to be covered
+ uint256 t0PoolDebt; // [WAD] t0 debt in pool after kick
+ uint256 t0KickedDebt; // [WAD] new t0 debt after kick
+ uint256 lup; // [WAD] current lup
 }
 
 struct SettleParams {
@@ -38,16 +37,16 @@ struct SettleParams {
 }
 
 struct TakeResult {
- uint256 collateralAmount;
- uint256 quoteTokenAmount;
- uint256 t0RepayAmount;
- uint256 t0DebtPenalty;
- uint256 excessQuoteToken;
- uint256 remainingCollateral;
- uint256 poolDebt;
- uint256 newLup;
- uint256 t0DebtInAuctionChange;
- bool settledAuction;
+ uint256 collateralAmount; // [WAD] amount of collateral taken
+ uint256 quoteTokenAmount; // [WAD] amount of quote tokens paid by taker for taken collateral
+ uint256 t0DebtPenalty; // [WAD] t0 penalty applied on first take
+ uint256 excessQuoteToken; // [WAD] (NFT only) amount of quote tokens to be paid by taker to borrower for fractional collateral
+ uint256 remainingCollateral; // [WAD] amount of borrower collateral remaining after take
+ uint256 poolDebt; // [WAD] current pool debt
+ uint256 t0PoolDebt; // [WAD] t0 pool debt
+ uint256 newLup; // [WAD] current lup
+ uint256 t0DebtInAuctionChange; // [WAD] the amount of t0 debt recovered by take action
+ bool settledAuction; // true if auction is settled by take action
 }
 
 /******************************************/
@@ -83,7 +82,7 @@ struct DrawDebtResult {
  uint256 remainingCollateral; // [WAD] amount of borrower collateral after draw debt (for NFT can be diminished if auction settled)
  bool settledAuction; // true if collateral pledged settles auction
  uint256 t0DebtInAuctionChange; // [WAD] change of t0 pool debt in auction after pledge collateral
- uint256 t0DebtChange; // [WAD] change of total t0 pool debt after after draw debt
+ uint256 t0PoolDebt;  // [WAD] amount of t0 debt in pool after draw debt
 }
 
 struct RepayDebtResult {
@@ -93,6 +92,6 @@ struct RepayDebtResult {
  uint256 remainingCollateral; // [WAD] amount of borrower collateral after pull collateral
  bool settledAuction; // true if repay debt settles auction
  uint256 t0DebtInAuctionChange; // [WAD] change of t0 pool debt in auction after repay debt
- uint256 t0RepaidDebt; // [WAD] amount of t0 repaid debt
+ uint256 t0PoolDebt;  // [WAD] amount of t0 debt in pool after repay
  uint256 quoteTokenToRepay; // [WAD] quote token amount to be transferred from sender to pool
 }
diff --git a/src/interfaces/pool/commons/IPoolReserveAuctionActions.sol b/src/interfaces/pool/commons/IPoolReserveAuctionActions.sol
@@ -27,7 +27,7 @@ interface IPoolReserveAuctionActions {
 
 struct StartReserveAuctionParams {
  uint256 poolSize; // [WAD] total deposits in pool (with accrued debt)
- uint256 poolDebt;  // [WAD] current t0 pool debt
+ uint256 t0PoolDebt; // [WAD] current t0 pool debt
  uint256 poolBalance; // [WAD] pool quote token balance
  uint256 inflator; // [WAD] pool current inflator
 }
diff --git a/src/interfaces/pool/commons/IPoolState.sol b/src/interfaces/pool/commons/IPoolState.sol
@@ -250,6 +250,7 @@ struct PoolBalancesState {
 
 struct PoolState {
  uint8 poolType; // pool type, can be ERC20 or ERC721
+ uint256 t0Debt; // [WAD] t0 debt in pool
  uint256 debt; // [WAD] total debt in pool, accrued in current block
  uint256 collateral; // [WAD] total collateral pledged in pool
  uint256 inflator; // [WAD] current pool inflator