Summary
The GlobalConfiguration::removeCollateralFromLiquidationPriority function removes any collateral of choice from the collateralLiquidationPriority array. In order to achieve this, the function loops through the array, copies each element to a different array neglecting the element to be removed. While copying the elements, all elements are removed from the collateralLiquidationPriority array. After all elements have been removed from the collateralLiquidationPriority array in storage, the other elements earlier copied to a different array are then added to the collateralLiquidationPriority array in storage one after the other in order to maintain the initial order of the elements instead of changing the order as is the case in openzeppelin's EnumerableSets. This creates a potential denial of service (DoS), continuously incrementing gas costs as the array gets larger.
Vulnerability Details
The GlobalConfiguration::removeCollateralFromLiquidationPriority function loops through the GlobalConfiguration::collateralLiquidationPriority array, copying each collateralType in the array one after the other into a new array copyCollateralLiquidationPriority except the collateralType to be removed. Hereafter, each collateralType in the copyCollateralLiquidationPriority is added into the collateralLiquidationPriority array in storage. However, the longer the GlobalConfiguration::removeCollateralFromLiquidationPriority array is, the more collateralTypes involved in the looping. This means the gas costs for removing a collateralType when the GlobalConfiguration::collateralLiquidationPriority is small in size is way less than when the GlobalConfiguration::collateralLiquidationPriority is larger in size.
uint256 copyCollateralLiquidationPriorityLength = copyCollateralLiquidationPriority.length;
address[] memory newCollateralLiquidationPriority = new address[](copyCollateralLiquidationPriorityLength - 1);
uint256 indexCollateral;
for (uint256 i; i < copyCollateralLiquidationPriorityLength; i++) {
address collateral = copyCollateralLiquidationPriority[i];
self.collateralLiquidationPriority.remove(collateral);
if (collateral == collateralType) {
continue;
}
newCollateralLiquidationPriority[indexCollateral] = collateral;
indexCollateral++;
}
for (uint256 i; i < copyCollateralLiquidationPriorityLength - 1; i++) {
self.collateralLiquidationPriority.add(newCollateralLiquidationPriority[i]);
}
Impact
The gas costs for removing a collateralType will greatly increase as more collateralTypes are added into the collateralLiquidationPriority array, making it difficult to remove collaterals from the collateralLiquidationPriority array in the long run.
Tools Used
Manual Review
Foundry
Proof of Concept: Consider having two arrays of collateralTypes, one of size 2 and the second of size 50. We first add the first array to the collateralLiquidationPriority array and then remove one collateralType from the collateralLiquidationPriority. Secondly, add the second array to the collateralLiquidationPriority array and then remove one collateralType from the collateralLiquidationPriority. The gas costs will be as such:
This is about 16 times more expensive removing from the array when it is larger.
PoC
Place the following code into `removeCollateralFromLiquidationPriority.t.sol`.
function test_RemoveCollateralFromLiquidationPriority_LeadsToDoS() external {
vm.txGasPrice(1);
address collateralType1 = address(123);
address collateralType2 = address(456);
address[] memory collateralTypes = new address[](2);
collateralTypes[0] = collateralType1;
collateralTypes[1] = collateralType2;
perpsEngine.exposed_configureCollateralLiquidationPriority(collateralTypes);
uint256 gasBeforeFirstRemoval = gasleft();
perpsEngine.exposed_removeCollateralFromLiquidationPriority(collateralType1);
uint256 gasAfterFirstRemoval = gasleft();
uint256 firstRemovalGas = (gasBeforeFirstRemoval - gasAfterFirstRemoval)*tx.gasprice;
uint160 newLength = 50;
address[] memory otherCollateralTypes = new address[](newLength);
uint160 i;
for(i = 1; i <= newLength; i++){
address newCollateral = address(i);
otherCollateralTypes[i-1] = newCollateral;
}
perpsEngine.exposed_configureCollateralLiquidationPriority(otherCollateralTypes);
address otherCollateralTypes1 = otherCollateralTypes[1];
uint256 gasBeforeSecondRemoval = gasleft();
perpsEngine.exposed_removeCollateralFromLiquidationPriority(otherCollateralTypes1);
uint256 gasAfterSecondRemoval = gasleft();
uint256 secondRemovalGas = (gasBeforeSecondRemoval - gasAfterSecondRemoval)*tx.gasprice;
console2.log("First Removal Gas Used", firstRemovalGas);
console2.log("Second Removal Gas Used", secondRemovalGas);
assertGt(secondRemovalGas, firstRemovalGas);
}
Recommendations
Consider using the remove function from openzeppelin's EnumerableSets without rearranging the array to maintain its original order. Though, the order may have changed, each collateralType can easily be accessed.
function removeCollateralFromLiquidationPriority(Data storage self, address collateralType) internal {
// does the collateral being removed exist?
bool isInCollateralLiquidationPriority = self.collateralLiquidationPriority.contains(collateralType);
// if not, revert
if (!isInCollateralLiquidationPriority) revert Errors.MarginCollateralTypeNotInPriority(collateralType);
+ self.collateralLiquidationPriority.remove(collateral);
- // the following code is required since EnumerableSet::remove
- // uses the swap-and-pop technique which corrupts the order
- // copy the existing collateral order
- address[] memory copyCollateralLiquidationPriority = self.collateralLiquidationPriority.values();
- // cache length
- uint256 copyCollateralLiquidationPriorityLength = copyCollateralLiquidationPriority.length;
- // create a new array to store the new order
- address[] memory newCollateralLiquidationPriority = new address[](copyCollateralLiquidationPriorityLength - 1);
- uint256 indexCollateral;
- // iterate over the in-memory copies
- for (uint256 i; i < copyCollateralLiquidationPriorityLength; i++) {
- // fetch current collateral
- address collateral = copyCollateralLiquidationPriority[i];
- // remove current collateral from storage set
- self.collateralLiquidationPriority.remove(collateral);
- // if the current collateral is the one we want to remove, skip
- // to the next loop iteration
- if (collateral == collateralType) {
- continue;
- }
- // otherwise add current collateral to the new in-memory
- // order we are building
- newCollateralLiquidationPriority[indexCollateral] = collateral;
- indexCollateral++;
- }
- // the collateral priority in storage has been emptied and the new
- // order has been built in memory, so iterate through the new order
- // and add it to storage
- for (uint256 i; i < copyCollateralLiquidationPriorityLength - 1; i++) {
- self.collateralLiquidationPriority.add(newCollateralLiquidationPriority[i]);
- }
}
