-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
eeddf7e
commit 88e4710
Showing
1,024 changed files
with
91,222 additions
and
10 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
Silly Flaxen Goose | ||
|
||
High | ||
|
||
# Insufficient checks to confirm the correct status of `sequencerUptimeFeed` in `DebitaChainlink.sol` | ||
|
||
### Summary | ||
|
||
The missing check for a `0` value in `sequencerUptimeFeed.startedAt` will cause inaccurate sequencer status validation for the Debita platform as `getThePrice()` will pass incorrectly when `startedAt` is `0` and answer is also `0`, failing to validate the sequencer status effectively. | ||
|
||
### Root Cause | ||
|
||
In [DebitaChainlink.sol:61](https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/376fec45be95bd4bbc929fd37b485076b03ab8b0/Debita-V3-Contracts/contracts/oracles/DebitaChainlink.sol#L61), the lack of a `startedAt != 0` check in `checkSequencer()` fails to confirm an updated sequencer status during invalid rounds. | ||
|
||
Making sure `startedAt` isn't `0` is crucial for keeping the system secure and properly informed about the sequencer's status. | ||
|
||
### Internal pre-conditions | ||
|
||
1. `checkSequencer()` to be called within `getThePrice()`. | ||
2. `sequencerUptimeFeed` to have both answer and `startedAt` set to `0`. | ||
|
||
### External pre-conditions | ||
|
||
1. experiencing a brief downtime with delayed updates in Chainlink's L2 uptime feed. | ||
2. Invalid round leading to a `startedAt` value of 0. | ||
|
||
### Attack Path | ||
|
||
1. The sequencer feed returns `answer == 0` and `startedAt == 0` due to invalid round. | ||
2. `checkSequencer()` executes without `startedAt` check, passing verification even though sequencer status is unconfirmed. | ||
|
||
### Impact | ||
|
||
Debita suffers an approximate security vulnerability, as the contract mistakenly assumes sequencer uptime, exposing protocol to outdated or incorrect oracle data. | ||
|
||
FYR: Chainlink https://github.com/smartcontractkit/documentation/pull/1995 | ||
|
||
|
||
### PoC | ||
|
||
A recent [pull request](https://github.com/smartcontractkit/documentation/pull/1995) to update the [chainlink docs](https://docs.chain.link/data-feeds/l2-sequencer-feeds) | ||
|
||
### Mitigation | ||
|
||
Add a `require(startedAt != 0, "Invalid sequencer status");` check in `checkSequencer()`. | ||
|
||
DebitaChainlink.sol | ||
|
||
```diff | ||
|
||
DebitaChainlink.sol | ||
|
||
|
||
function checkSequencer() public view returns (bool) { | ||
(, int256 answer, uint256 startedAt, , ) = sequencerUptimeFeed.latestRoundData(); | ||
|
||
// Check if the sequencer is up | ||
bool isSequencerUp = answer == 0; | ||
if (!isSequencerUp) { | ||
revert SequencerDown(); | ||
} | ||
|
||
+ // Ensure that startedAt is valid and non-zero | ||
+ require(startedAt != 0, "Invalid sequencer status"); | ||
|
||
// Calculate the time since the sequencer came back up | ||
uint256 timeSinceUp = block.timestamp - startedAt; | ||
if (timeSinceUp <= GRACE_PERIOD_TIME) { | ||
revert GracePeriodNotOver(); | ||
} | ||
|
||
return true; | ||
} | ||
``` | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
Happy Rouge Coyote | ||
|
||
Medium | ||
|
||
# Index conflict in _deleteAuctionOrder: Deleting Auction Order Ambiguity | ||
|
||
### Summary | ||
|
||
The [`_deleteAuctionOrder`](https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/auctions/AuctionFactory.sol#L145) and every delete functions of `Factory` Contracts are designed to remove an active implementation, identified by its address, from the `allActiveOrders` array and update the corresponding index mappings. The function is intended to keep track of active orders by re-indexing them in a way that maintains order integrity while reducing the `activeOrdersCount`. | ||
|
||
### Root Cause | ||
|
||
Here is an example of `AuctionFactory` Contract which uses the same logic for evey factory contract of protocol | ||
|
||
The first creator of auction will get the 0th index of the `AuctionOrderIndex` mapping: | ||
|
||
```solidity | ||
function createAuction( | ||
uint _veNFTID, | ||
address _veNFTAddress, | ||
address liquidationToken, | ||
uint _initAmount, | ||
uint _floorAmount, | ||
uint _duration | ||
) public returns (address) { | ||
... | ||
// LOGIC INDEX | ||
AuctionOrderIndex[address(_createdAuction)] = activeOrdersCount; | ||
... | ||
} | ||
``` | ||
|
||
`activeOrdersCount` initially is set to 0 at declaration. Then at the `_deleteAuctionOrder` function sets the auction address to 0th index assuming that 0 is not used as valid index. | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
1. This conflict can lead to incorrect behavior when other functions try to retrieve the index of auction orders, as they may mistakenly interpret a `0` index as either an active order at index `0` or a deleted order. This can lead to data inconsistencies. | ||
|
||
2. Other functions that rely on AuctionOrderIndex to access or manage active orders may misinterpret a deleted order as an active one, causing unwanted errors or undefined behavior in the contract. | ||
|
||
### PoC | ||
|
||
```solidity | ||
function testCreateAuction() public { | ||
vm.startPrank(seller); | ||
veNFTContract.approve(address(factory), 1); | ||
auction = factory.createAuction(1, address(veNFTContract), address(AERO), 100, 50, 100); | ||
vm.stopPrank(); | ||
uint index = factory.AuctionOrderIndex(auction); | ||
console.log("index of first auction %s", index); | ||
assertTrue(factory.isAuction(auction)); | ||
address secondSeller = makeAddr("secondSeller"); | ||
veNFTContract.mint(secondSeller, 2); | ||
vm.startPrank(secondSeller); | ||
veNFTContract.approve(address(factory), 2); | ||
address secondAuction = factory.createAuction(2, address(veNFTContract), address(AERO), 100, 50, 100); | ||
vm.stopPrank(); | ||
uint index2 = factory.AuctionOrderIndex(secondAuction); | ||
console.log("index of second auction %s", index2); | ||
vm.prank(secondSeller); | ||
DutchAuction_veNFT(secondAuction).cancelAuction(); | ||
index2 = factory.AuctionOrderIndex(secondAuction); | ||
console.log("index of second auction after cancel %s", index2); | ||
} | ||
``` | ||
|
||
```plain | ||
Logs: | ||
index of first auction 0 | ||
index of second auction 1 | ||
index of second auction after cancel 0 | ||
``` | ||
|
||
_No response_ | ||
|
||
### Mitigation | ||
|
||
Increment `activeOrdersCount` at the start of `createAuction` function to skip the 0 and reserve it for non-exisiting auctions. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
Happy Rouge Coyote | ||
|
||
Medium | ||
|
||
# Delete functions lacks mapping updates | ||
|
||
### Summary | ||
|
||
The [`_deleteAuctionOrder`](https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/auctions/AuctionFactory.sol#L145) function aims to remove an auction order from the active orders list and update necessary mappings to reflect the deletion. However, there is an overlooked issue: the function does not update the `isAuction` mapping to reflect that the auction is no longer active. | ||
|
||
The [`deleteBorrowOffer`](https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/DebitaBorrowOffer-Factory.sol#L162) is also aims to remove an borrow order but does not update `isBorrowOrderLegit` mapping. | ||
|
||
The [`deleteOrder`](https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/main/Debita-V3-Contracts/contracts/DebitaLendOfferFactory.sol#L207) lacks updates on `isLendOrderLegit` mapping | ||
|
||
### Root Cause | ||
|
||
_No response_ | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
Giving example for first case. | ||
|
||
The `_deleteAuctionOrder` function does not reset `isAuction[_AuctionOrder]` to `false` upon deletion. This means that even after an auction order is deleted, `isAuction[_AuctionOrder]` will remain `true`, inaccurately marking the auction order as active in the system. | ||
|
||
### PoC | ||
|
||
_No response_ | ||
|
||
### Mitigation | ||
|
||
Giving example for first case. | ||
|
||
Add missing sets to the `_deleteAuctionOrder` function | ||
|
||
```diff | ||
function _deleteAuctionOrder(address _AuctionOrder) external onlyAuctions { | ||
// get index of the Auction order | ||
uint index = AuctionOrderIndex[_AuctionOrder]; | ||
AuctionOrderIndex[_AuctionOrder] = 0; | ||
|
||
// get last Auction order | ||
allActiveAuctionOrders[index] = allActiveAuctionOrders[ | ||
activeOrdersCount - 1 | ||
]; | ||
// take out last Auction order | ||
allActiveAuctionOrders[activeOrdersCount - 1] = address(0); | ||
|
||
// switch index of the last Auction order to the deleted Auction order | ||
AuctionOrderIndex[allActiveAuctionOrders[index]] = index; | ||
activeOrdersCount--; | ||
+ isAuction[_AuctionOrder] = false; | ||
} | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
Deep Orange Bison | ||
|
||
Medium | ||
|
||
# Not Checking For Stale Prices | ||
|
||
### Summary | ||
|
||
Most prices are provided by an off-chain oracle archive via signed prices, but a Chainlink oracle is still used for index prices. These prices are insufficiently validated. | ||
|
||
### Root Cause | ||
|
||
```Solidity | ||
function getThePrice(address tokenAddress) public view returns (int) { | ||
// falta hacer un chequeo para las l2 | ||
address _priceFeed = priceFeeds[tokenAddress]; | ||
require(!isPaused, "Contract is paused"); | ||
require(_priceFeed != address(0), "Price feed not set"); | ||
AggregatorV3Interface priceFeed = AggregatorV3Interface(_priceFeed); | ||
// if sequencer is set, check if it's up | ||
// if it's down, revert | ||
if (address(sequencerUptimeFeed) != address(0)) { | ||
checkSequencer(); | ||
} | ||
(, int price, , , ) = priceFeed.latestRoundData(); // <- here | ||
require(isFeedAvailable[_priceFeed], "Price feed not available"); | ||
require(price > 0, "Invalid price"); | ||
return price; | ||
} | ||
``` | ||
|
||
This function doesn't check for updatedAt with his own heartbeat. | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
The current implementation of DebitaChainlink is used by the protocol to showcase how the feed will be retrieved via Chainlink Data Feeds. The feed is used to retrieve the getThePrice, which is also used afterwards by DebitaV3Aggregator.getPriceFrom(), then by many functions on protocol for create and matching borrowings, calculate collateral and other importants actions. | ||
|
||
### PoC | ||
|
||
Many smart contracts use Chainlink to request off-chain pricing data, but a common error occurs when the smart contract doesn’t check whether that data is stale. If the returned pricing data is stale, this code will execute with prices that don’t reflect the current pricing resulting in a potential loss of funds for the user and/or the protocol. Smart contracts should always check the updatedAt parameter returned from latestRoundData() and compare it to a staleness threshold: | ||
|
||
```Solidity | ||
(, int256 price, , uint256 updatedAt, ) = priceFeed.latestRoundData(); | ||
if (updatedAt + heartbeat < block.timestamp) { | ||
revert("stale price feed"); | ||
} | ||
``` | ||
|
||
The staleness threshold should correspond to the heartbeat of the oracle’s price feed. | ||
|
||
### Mitigation | ||
|
||
_No response_ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
Deep Orange Bison | ||
|
||
High | ||
|
||
# Assuming Oracle Price Precision | ||
|
||
### Summary | ||
|
||
The `matchOffersV3` function on DebitaV3Aggregator contract does not account for the decimals returned by the price feed from various oracles. This can lead to incorrect calculations of collateral-to-principal ratios (LTV), fees, and incentives, causing potential discrepancies in the protocol’s lending processes. This oversight introduces risks in how collateral and principal amounts are calculated, potentially impacting both the security and trustworthiness of the protocol. | ||
|
||
### Root Cause | ||
|
||
The root cause is the lack of normalization for price values returned by oracles, which may vary in decimal precision. Without adjusting the decimals, the function uses prices with inconsistent scales directly in calculations, resulting in inaccuracies in collateral and principal amounts. | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
If decimal discrepancies are ignored, LTV ratios can be miscalculated, affecting the collateral required for loans. This could allow undercollateralized loans, increasing the risk of losses in case of defaults. Misaligned decimals can cause errors in fee and incentive calculations, either overcharging users or reducing protocol revenue. Attackers could exploit these decimal inaccuracies to manipulate LTV calculations, potentially borrowing more than the fair collateral would allow. | ||
|
||
### PoC | ||
|
||
When using Oracle price feeds, developers must consider that different feeds can have varying decimal precision. Assuming uniform precision across feeds is a mistake, as not all price feeds follow the same standard. For instance, most non-ETH pairs typically use 8 decimals, while ETH pairs usually use 18 decimals. However, exceptions exist—ETH/USD, considered a non-ETH pair, reports with 8 decimals, while some feeds like AMPL/USD use 18 decimals, contrary to the usual 8-decimal format for USD pairs. | ||
|
||
Smart contracts can use DebitaChainlink.getDecimals() to retrieve the exact decimal precision for each price feed, ensuring calculations are accurate across different feed formats. | ||
|
||
[matchOffersV3 function](https://github.com/sherlock-audit/2024-11-debita-finance-v3/blob/376fec45be95bd4bbc929fd37b485076b03ab8b0/Debita-V3-Contracts/contracts/DebitaV3Aggregator.sol#L274-L647) | ||
|
||
### Mitigation | ||
|
||
_No response_ |
Oops, something went wrong.