Platform: Code4rena
Start Date: 25/10/2022
Pot Size: $50,000 USDC
Total HM: 18
Participants: 127
Period: 5 days
Judge: 0xean
Total Solo HM: 9
Id: 175
League: ETH
Rank: 2/127
Findings: 4
Award: $3,638.23
🌟 Selected for report: 3
🚀 Solo Findings: 1
203.1474 USDC - $203.15
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L559-L572 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L531-L539 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L472-L474 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L460-L466
When a user incurs a DBR deficit, a replenisher can call the forceReplenish
function to force the user to replenish DBR. However, there is no guarantee that the forceReplenish
function will always be called. When the forceReplenish
function is not called, such as because that the replenisher does not notice the user's DBR deficit promptly, the user can just call the repay
function to repay the origianl debt and the withdraw
function to receive all of the deposited collateral even when the user has a DBR deficit already. Yet, in the same situation, if the forceReplenish
function has been called, more debt should be added for the user, and the user needs to repay more in order to get back all of the deposited collateral. Hence, when the forceReplenish
function is not called while it could be called, the Market
contract would receive less DOLA if the user decides to repay the debt and withdraw the collateral both in full.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L559-L572
function forceReplenish(address user, uint amount) public { uint deficit = dbr.deficitOf(user); require(deficit > 0, "No DBR deficit"); require(deficit >= amount, "Amount > deficit"); uint replenishmentCost = amount * dbr.replenishmentPriceBps() / 10000; uint replenisherReward = replenishmentCost * replenishmentIncentiveBps / 10000; debts[user] += replenishmentCost; uint collateralValue = getCollateralValueInternal(user); require(collateralValue >= debts[user], "Exceeded collateral value"); totalDebt += replenishmentCost; dbr.onForceReplenish(user, amount); dola.transfer(msg.sender, replenisherReward); emit ForceReplenish(user, msg.sender, amount, replenishmentCost, replenisherReward); }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L531-L539
function repay(address user, uint amount) public { uint debt = debts[user]; require(debt >= amount, "Insufficient debt"); debts[user] -= amount; totalDebt -= amount; dbr.onRepay(user, amount); dola.transferFrom(msg.sender, address(this), amount); emit Repay(user, msg.sender, amount); }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L472-L474
function withdraw(uint amount) public { withdrawInternal(msg.sender, msg.sender, amount); }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L460-L466
function withdrawInternal(address from, address to, uint amount) internal { uint limit = getWithdrawalLimitInternal(from); require(limit >= amount, "Insufficient withdrawal limit"); IEscrow escrow = getEscrow(from); escrow.pay(to, amount); emit Withdraw(from, to, amount); }
Please add the following test in src\test\Market.t.sol
. This test will pass to demonstrate the described scenario.
function testRepayAndWithdrawInFullWhenIncurringDBRDeficitIfNotBeingForcedToReplenish() public { gibWeth(user, wethTestAmount); gibDBR(user, wethTestAmount); vm.startPrank(user); // user deposits wethTestAmount WETH and borrows wethTestAmount DOLA deposit(wethTestAmount); market.borrow(wethTestAmount); assertEq(DOLA.balanceOf(user), wethTestAmount); assertEq(WETH.balanceOf(user), 0); vm.warp(block.timestamp + 60 weeks); // after some time, user incurs DBR deficit assertGt(dbr.deficitOf(user), 0); // yet, since no one notices that user has a DBR deficit and forces user to replenish DBR, // user is able to repay wethTestAmount DOLA that was borrowed previously and withdraw wethTestAmount WETH that was deposited previously market.repay(user, wethTestAmount); market.withdraw(wethTestAmount); vm.stopPrank(); // as a result, user is able to get back all of the deposited WETH, which should not be possible if user has been forced to replenish DBR assertEq(DOLA.balanceOf(user), 0); assertEq(WETH.balanceOf(user), wethTestAmount); }
VSCode
When calling the repay
function, the user's DBR deficit can also be checked. If the user has a DBR deficit, an amount, which is similar to replenishmentCost
that is calculated in the forceReplenish
function, can be calculated; it can then be used to adjust the repay
function's amount
input for updating the states regarding the user's and total debts in the relevant contracts.
#0 - d3e4
2022-11-02T08:09:44Z
This seems to be a design choice. The user should perhaps be allowed to repay his debt without penalty if he does so quickly enough. That is, if forced replenishment is designed as a threat rather than an actual cost of being in deficit.
#1 - c4-judge
2022-11-05T23:06:07Z
0xean marked the issue as duplicate
#2 - c4-judge
2022-12-01T15:57:32Z
0xean marked the issue as selected for report
#3 - c4-judge
2022-12-07T08:15:30Z
Simon-Busch marked the issue as not a duplicate
#4 - c4-judge
2022-12-07T08:15:37Z
Simon-Busch marked the issue as primary issue
#5 - c4-sponsor
2022-12-14T14:19:21Z
08xmt marked the issue as sponsor disputed
#6 - 08xmt
2022-12-14T14:19:29Z
Working as intended.
🌟 Selected for report: rbserver
Also found by: 0x1f8b, 0xNazgul, 0xc0ffEE, 8olidity, Aymen0909, Chom, Franfran, Jeiwan, Jujic, Lambda, M4TZ1P, Olivierdem, Rolezn, Ruhum, TomJ, Wawrdog, __141345__, bin2chen, c7e7eff, carlitox477, catchup, cccz, codexploder, cuteboiz, d3e4, dipp, djxploit, eierina, elprofesor, hansfriese, horsefacts, idkwhatimdoing, imare, immeas, joestakey, ladboy233, leosathya, martin, minhtrng, pashov, peanuts, pedroais, rokinot, rvierdiiev, saneryee, sorrynotsorry, tonisives
0.5004 USDC - $0.50
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L78-L105 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L112-L144 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363
Calling the Oracle
contract's viewPrice
or getPrice
function executes uint price = feeds[token].feed.latestAnswer()
and require(price > 0, "Invalid feed price")
. Besides that Chainlink's latestAnswer
function is deprecated, only verifying that price > 0
is true is also not enough to guarantee that the returned price
is not stale. Using a stale price
can cause the calculations for the credit and withdrawal limits to be inaccurate, which, for example, can mistakenly consider a user's debt to be under water and unexpectedly allow the user's debt to be liquidated.
To avoid using a stale answer returned by the Chainlink oracle data feed, according to Chainlink's documentation:
latestRoundData
function can be used instead of the deprecated latestAnswer
function.roundId
and answeredInRound
are also returned. "You can check answeredInRound
against the current roundId
. If answeredInRound
is less than roundId
, the answer is being carried over. If answeredInRound
is equal to roundId
, then the answer is fresh."https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L78-L105
function viewPrice(address token, uint collateralFactorBps) external view returns (uint) { if(fixedPrices[token] > 0) return fixedPrices[token]; if(feeds[token].feed != IChainlinkFeed(address(0))) { // get price from feed uint price = feeds[token].feed.latestAnswer(); require(price > 0, "Invalid feed price"); // normalize price uint8 feedDecimals = feeds[token].feed.decimals(); uint8 tokenDecimals = feeds[token].tokenDecimals; uint8 decimals = 36 - feedDecimals - tokenDecimals; uint normalizedPrice = price * (10 ** decimals); uint day = block.timestamp / 1 days; // get today's low uint todaysLow = dailyLows[token][day]; // get yesterday's low uint yesterdaysLow = dailyLows[token][day - 1]; // calculate new borrowing power based on collateral factor uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow; if(twoDayLow > 0 && newBorrowingPower > twoDayLow) { uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps; return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice; } return normalizedPrice; } revert("Price not found"); }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L112-L144
function getPrice(address token, uint collateralFactorBps) external returns (uint) { if(fixedPrices[token] > 0) return fixedPrices[token]; if(feeds[token].feed != IChainlinkFeed(address(0))) { // get price from feed uint price = feeds[token].feed.latestAnswer(); require(price > 0, "Invalid feed price"); // normalize price uint8 feedDecimals = feeds[token].feed.decimals(); uint8 tokenDecimals = feeds[token].tokenDecimals; uint8 decimals = 36 - feedDecimals - tokenDecimals; uint normalizedPrice = price * (10 ** decimals); // potentially store price as today's low uint day = block.timestamp / 1 days; uint todaysLow = dailyLows[token][day]; if(todaysLow == 0 || normalizedPrice < todaysLow) { dailyLows[token][day] = normalizedPrice; todaysLow = normalizedPrice; emit RecordDailyLow(token, normalizedPrice); } // get yesterday's low uint yesterdaysLow = dailyLows[token][day - 1]; // calculate new borrowing power based on collateral factor uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow; if(twoDayLow > 0 && newBorrowingPower > twoDayLow) { uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps; return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice; } return normalizedPrice; } revert("Price not found"); }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347
function getCreditLimitInternal(address user) internal returns (uint) { uint collateralValue = getCollateralValueInternal(user); return collateralValue * collateralFactorBps / 10000; }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327
function getCollateralValueInternal(address user) internal returns (uint) { IEscrow escrow = predictEscrow(user); uint collateralBalance = escrow.balance(); return collateralBalance * oracle.getPrice(address(collateral), collateralFactorBps) / 1 ether; }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363
function getWithdrawalLimitInternal(address user) internal returns (uint) { IEscrow escrow = predictEscrow(user); uint collateralBalance = escrow.balance(); if(collateralBalance == 0) return 0; uint debt = debts[user]; if(debt == 0) return collateralBalance; if(collateralFactorBps == 0) return 0; uint minimumCollateral = debt * 1 ether / oracle.getPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps; if(collateralBalance <= minimumCollateral) return 0; return collateralBalance - minimumCollateral; }
The following steps can occur for the described scenario.
depositAndBorrow
function to deposit some WETH as the collateral and borrows some DOLA against the collateral.liquidate
function for trying to liquidate Alice's debt. Because the Chainlink oracle data feed returns an up-to-date price at this moment, the getCreditLimitInternal
function calculates Alice's credit limit accurately, which does not cause Alice's debt to be under water. Hence, Bob's liquidate
transaction reverts.liquidate
function again for trying to liquidate Alice's debt. This time, because the Chainlink oracle data feed returns a positive but stale price, the getCreditLimitInternal
function calculates Alice's credit limit inaccurately, which mistakenly causes Alice's debt to be under water.liquidate
transaction is executed successfully so he gains some of Alice's WETH collateral. Alice loses such WETH collateral amount unexpectedly because her debt should not be considered as under water if the stale price was not used.VSCode
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L82-L83 and https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L116-L117 can be updated to the following code.
(uint80 roundId, int256 answer, , uint256 updatedAt, uint80 answeredInRound) = feeds[token].feed.latestRoundData(); require(answeredInRound >= roundId, "answer is stale"); require(updatedAt > 0, "round is incomplete"); require(answer > 0, "Invalid feed answer"); uint256 price = uint256(answer);
#0 - neumoxx
2022-10-31T08:40:48Z
Duplicate of #601
#1 - c4-judge
2022-11-05T17:48:17Z
0xean marked the issue as duplicate
#2 - c4-judge
2022-12-03T20:32:40Z
0xean marked the issue as selected for report
#3 - c4-judge
2022-12-07T08:13:18Z
Simon-Busch marked the issue as not a duplicate
#4 - c4-judge
2022-12-07T08:13:26Z
Simon-Busch marked the issue as primary issue
#5 - 08xmt
2022-12-14T14:21:10Z
#6 - c4-sponsor
2022-12-14T14:21:19Z
08xmt marked the issue as sponsor confirmed
🌟 Selected for report: rbserver
3397.8465 USDC - $3,397.85
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L78-L105 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L112-L144 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363
Based on the current implementation, when the protocol wants to use Chainlink oracle data feed for getting a collateral token's price, the fixed price for the token should not be set. When the fixed price is not set for the token, calling the Oracle
contract's viewPrice
or getPrice
function will execute uint price = feeds[token].feed.latestAnswer()
. As https://blog.openzeppelin.com/secure-smart-contract-guidelines-the-dangers-of-price-oracles/ mentions, it is possible that Chainlink’s "multisigs can immediately block access to price feeds at will". When this occurs, executing feeds[token].feed.latestAnswer()
will revert so calling the viewPrice
and getPrice
functions also revert, which cause denial of service when calling functions like getCollateralValueInternal
andgetWithdrawalLimitInternal
. The getCollateralValueInternal
andgetWithdrawalLimitInternal
functions are the key elements to the core functionalities, such as borrowing, withdrawing, force-replenishing, and liquidating; with these functionalities facing DOS, the protocol's usability becomes very limited.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L78-L105
function viewPrice(address token, uint collateralFactorBps) external view returns (uint) { if(fixedPrices[token] > 0) return fixedPrices[token]; if(feeds[token].feed != IChainlinkFeed(address(0))) { // get price from feed uint price = feeds[token].feed.latestAnswer(); require(price > 0, "Invalid feed price"); // normalize price uint8 feedDecimals = feeds[token].feed.decimals(); uint8 tokenDecimals = feeds[token].tokenDecimals; uint8 decimals = 36 - feedDecimals - tokenDecimals; uint normalizedPrice = price * (10 ** decimals); uint day = block.timestamp / 1 days; // get today's low uint todaysLow = dailyLows[token][day]; // get yesterday's low uint yesterdaysLow = dailyLows[token][day - 1]; // calculate new borrowing power based on collateral factor uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow; if(twoDayLow > 0 && newBorrowingPower > twoDayLow) { uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps; return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice; } return normalizedPrice; } revert("Price not found"); }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L112-L144
function getPrice(address token, uint collateralFactorBps) external returns (uint) { if(fixedPrices[token] > 0) return fixedPrices[token]; if(feeds[token].feed != IChainlinkFeed(address(0))) { // get price from feed uint price = feeds[token].feed.latestAnswer(); require(price > 0, "Invalid feed price"); // normalize price uint8 feedDecimals = feeds[token].feed.decimals(); uint8 tokenDecimals = feeds[token].tokenDecimals; uint8 decimals = 36 - feedDecimals - tokenDecimals; uint normalizedPrice = price * (10 ** decimals); // potentially store price as today's low uint day = block.timestamp / 1 days; uint todaysLow = dailyLows[token][day]; if(todaysLow == 0 || normalizedPrice < todaysLow) { dailyLows[token][day] = normalizedPrice; todaysLow = normalizedPrice; emit RecordDailyLow(token, normalizedPrice); } // get yesterday's low uint yesterdaysLow = dailyLows[token][day - 1]; // calculate new borrowing power based on collateral factor uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow; if(twoDayLow > 0 && newBorrowingPower > twoDayLow) { uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps; return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice; } return normalizedPrice; } revert("Price not found"); }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L344-L347
function getCreditLimitInternal(address user) internal returns (uint) { uint collateralValue = getCollateralValueInternal(user); return collateralValue * collateralFactorBps / 10000; }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L323-L327
function getCollateralValueInternal(address user) internal returns (uint) { IEscrow escrow = predictEscrow(user); uint collateralBalance = escrow.balance(); return collateralBalance * oracle.getPrice(address(collateral), collateralFactorBps) / 1 ether; }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L353-L363
function getWithdrawalLimitInternal(address user) internal returns (uint) { IEscrow escrow = predictEscrow(user); uint collateralBalance = escrow.balance(); if(collateralBalance == 0) return 0; uint debt = debts[user]; if(debt == 0) return collateralBalance; if(collateralFactorBps == 0) return 0; uint minimumCollateral = debt * 1 ether / oracle.getPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps; if(collateralBalance <= minimumCollateral) return 0; return collateralBalance - minimumCollateral; }
The following steps can occur for the described scenario.
depositAndBorrow
function to deposit some of the collateral token and borrows some DOLA against the collateral.feeds[token].feed.latestAnswer()
will revert.borrow
function, which eventually executes feeds[token].feed.latestAnswer()
, reverts.withdraw
function, which eventually executes feeds[token].feed.latestAnswer()
, reverts.forceReplenish
and liquidate
functions would all revert as well.VSCode
The Oracle
contract's viewPrice
and getPrice
functions can be updated to refactor feeds[token].feed.latestAnswer()
into try feeds[token].feed.latestAnswer() returns (int256 price) { ... } catch Error(string memory) { ... }
. The logic for getting the collateral token's price from the Chainlink oracle data feed should be placed in the try
block while some fallback logic when the access to the Chainlink oracle data feed is denied should be placed in the catch
block. If getting the fixed price for the collateral token is considered as a fallback logic, then setting the fixed price for the token should become mandatory, which is different from the current implementation. Otherwise, fallback logic for getting the token's price from a fallback oracle is needed.
#0 - c4-sponsor
2022-11-14T08:58:47Z
08xmt marked the issue as disagree with severity
#1 - c4-sponsor
2022-11-14T08:58:54Z
08xmt marked the issue as sponsor acknowledged
#2 - 08xmt
2022-11-14T09:00:33Z
In the unlikely event of a chainlink msig block, the protocol can still recover through the use of governance actions to insert a new feed. I'd consider this a Low Severity, as protocol is only DOS'ed for a short period, and can't be repeatedly DOS'ed.
#3 - 0xean
2022-11-28T18:03:07Z
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
I don't think a M requires some amount of time for the DOS to be valid, so I think without a mitigation or fallback in place, this is a valid issue and should qualify as M
#4 - c4-judge
2022-11-28T19:35:19Z
0xean marked the issue as satisfactory
#5 - 08xmt
2022-11-30T11:30:54Z
@0xean That's fair.
#6 - c4-judge
2022-12-01T15:53:20Z
0xean marked the issue as selected for report
🌟 Selected for report: 0x1f8b
Also found by: 0xNazgul, 0xSmartContract, Aymen0909, B2, Bnke0x0, Deivitto, Diana, Dinesh11G, ElKu, JC, Josiah, Rahoz, RaymondFam, ReyAdmirado, Rolezn, Waze, __141345__, adriro, aphak5010, brgltd, c3phas, c7e7eff, carlitox477, cducrest, ch0bu, chrisdior4, cryptonue, cryptostellar5, cylzxje, d3e4, delfin454000, enckrish, evmwanderer, fatherOfBlocks, gogo, hansfriese, horsefacts, immeas, leosathya, lukris02, neumo, oyc_109, pedr02b2, rbserver, robee, rotcivegaf, rvierdiiev, sakshamguruji, shark, simon135, tnevler, trustindistrust, wagmi
36.7345 USDC - $36.73
constructor
AND SETTERSSome state variables have different restrictions when setting them in constructor
and the corresponding setter functions. It is possible that the values, which can be set in the constructor
, cannot be set in the setter functions or vice versa. These inconsistencies can confuse users and developers. For better user experience and maintainability, please consider updating the restrictions for the following state variables to be the same in constructor
and the corresponding setter functions.
replenishmentPriceBps
's setter function requires that newReplenishmentPriceBps_ > 0
but constructor
does not require that at all.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L30-L42
constructor( uint _replenishmentPriceBps, string memory _name, string memory _symbol, address _operator ) { replenishmentPriceBps = _replenishmentPriceBps; ... }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L62-L65
function setReplenishmentPriceBps(uint newReplenishmentPriceBps_) public onlyOperator { require(newReplenishmentPriceBps_ > 0, "replenishment price must be over 0"); replenishmentPriceBps = newReplenishmentPriceBps_; }
replenishmentIncentiveBps
's setter function requires that _replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000
but constructor
only requires _replenishmentIncentiveBps < 10000
.
liquidationIncentiveBps
's setter function requires that _liquidationIncentiveBps + liquidationFeeBps < 10000
but constructor
only requires that _liquidationIncentiveBps < 10000
.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L61-L90
constructor ( address _gov, address _lender, address _pauseGuardian, address _escrowImplementation, IDolaBorrowingRights _dbr, IERC20 _collateral, IOracle _oracle, uint _collateralFactorBps, uint _replenishmentIncentiveBps, uint _liquidationIncentiveBps, bool _callOnDepositCallback ) { require(_collateralFactorBps < 10000, "Invalid collateral factor"); require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps < 10000, "Invalid liquidation incentive"); require(_replenishmentIncentiveBps < 10000, "Replenishment incentive must be less than 100%"); ... collateralFactorBps = _collateralFactorBps; replenishmentIncentiveBps = _replenishmentIncentiveBps; liquidationIncentiveBps = _liquidationIncentiveBps; ... }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L172-L175
function setReplenismentIncentiveBps(uint _replenishmentIncentiveBps) public onlyGov { require(_replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000, "Invalid replenishment incentive"); replenishmentIncentiveBps = _replenishmentIncentiveBps; }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L183-L186
function setLiquidationIncentiveBps(uint _liquidationIncentiveBps) public onlyGov { require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps + liquidationFeeBps < 10000, "Invalid liquidation incentive"); liquidationIncentiveBps = _liquidationIncentiveBps; }
gov
, lender
, and pauseGuardian
The following setGov
, setLender
, and setPauseGuardian
functions can be called to set gov
, lender
, and pauseGuardian
. Because these functions do not include a two-step procedure, it is possible to assign these roles to wrong addresses. When this occurs, the functions that are only callable by these roles can become no-ops or be maliciously called. To reduce the potential attack surface, please consider implementing a two-step procedure for assigning each of these roles.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L130
function setGov(address _gov) public onlyGov { gov = _gov; }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L136
function setLender(address _lender) public onlyGov { lender = _lender; }
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L142
function setPauseGuardian(address _pauseGuardian) public onlyGov { pauseGuardian = _pauseGuardian; }
address(0)
CHECKS FOR CRITICAL ADDRESS INPUTSTo prevent unintended behaviors, critical addresses inputs should be checked against address(0)
.
Please consider checking _operator
in the following constructor
.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L30-L42
constructor( uint _replenishmentPriceBps, string memory _name, string memory _symbol, address _operator ) { ... operator = _operator; ... }
Please consider checking _gov
, _lender
, _pauseGuardian
, and _escrowImplementation
in the following constructor
.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L61-L90
constructor ( address _gov, address _lender, address _pauseGuardian, address _escrowImplementation, ... ) { ... gov = _gov; lender = _lender; pauseGuardian = _pauseGuardian; escrowImplementation = _escrowImplementation; ... }
Please consider checking _operator
in the following constructor
.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L29-L33
constructor( address _operator ) { operator = _operator; }
A comment regarding todo indicates that there is an unresolved action item for implementation, which needs to be addressed before the protocol deployment. Please review the todo comment in the following code.
https://github.com/code-423n4/2022-10-inverse/blob/main/src/escrows/INVEscrow.sol#L34-L36
constructor(IXINV _xINV) { xINV = _xINV; // TODO: Test whether an immutable variable will persist across proxies }
The phrase in the following comment should be "the user" instead of "th user". https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L128
@notice Get the DBR deficit of an address. Will return 0 if th user has zero DBR or more.
The word in the following comment should be "debt" instead of "deb". https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L129
@dev The deficit of a user is calculated as the difference between the user's accrued DBR deb + due DBR debt and their balance.
The phrase in the following comment should be "The amount" instead of "Th amount". https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L589
@param repaidDebt Th amount of user user debt to liquidate.
To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic numbers, such as 10000
, in the following code with constants.
src\DBR.sol 122: uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days; 135: uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days; 330: uint replenishmentCost = amount * replenishmentPriceBps / 10000; src\Market.sol 75: require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps < 10000, "Invalid liquidation incentive"); 150: require(_collateralFactorBps < 10000, "Invalid collateral factor"); 162: require(_liquidationFactorBps > 0 && _liquidationFactorBps <= 10000, "Invalid liquidation factor"); 173: require(_replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000, "Invalid replenishment incentive"); 184: require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps + liquidationFeeBps < 10000, "Invalid liquidation incentive"); 195: require(_liquidationFeeBps > 0 && _liquidationFeeBps + liquidationIncentiveBps < 10000, "Invalid liquidation fee"); 377: uint minimumCollateral = debt * 1 ether / oracle.viewPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps; 563: uint replenishmentCost = amount * dbr.replenishmentPriceBps() / 10000; 564: uint replenisherReward = replenishmentCost * replenishmentIncentiveBps / 10000; 583: return debt * liquidationFactorBps / 10000; src\Oracle.sol 87: uint8 decimals = 36 - feedDecimals - tokenDecimals; 89: uint day = block.timestamp / 1 days; 95: uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; 98: uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps; 121: uint8 decimals = 36 - feedDecimals - tokenDecimals; 124: uint day = block.timestamp / 1 days; 134: uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000; 137: uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
NatSpec comments provide rich code documentation. @param and/or @return comments are missing for the following functions. Please consider completing NatSpec comments for them.
src\Market.sol 226: function createEscrow(address user) internal returns (IEscrow instance) { 245: function getEscrow(address user) internal returns (IEscrow) { 292: function predictEscrow(address user) public view returns (IEscrow predicted) { 312: function getCollateralValue(address user) public view returns (uint) { 323: function getCollateralValueInternal(address user) internal returns (uint) { 334: function getCreditLimit(address user) public view returns (uint) { 344: function getCreditLimitInternal(address user) internal returns (uint) { 353: function getWithdrawalLimitInternal(address user) internal returns (uint) { 370: function getWithdrawalLimit(address user) public view returns (uint) { src\Oracle.sol 78: function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.
src\DBR.sol 262: function DOMAIN_SEPARATOR() public view virtual returns (bytes32) { 266: function computeDomainSeparator() internal view virtual returns (bytes32) { src\Market.sol 97: function DOMAIN_SEPARATOR() public view virtual returns (bytes32) { 101: function computeDomainSeparator() internal view virtual returns (bytes32) {
#0 - c4-judge
2022-11-08T00:52:31Z
0xean marked the issue as grade-b