DeFiFoundry
60,000 USDC
View results
Submission Details
Severity: low
Invalid

Inefficient `GlobalConfiguration::removeCollateralFromLiquidationPriority` function increases Gas Costs and reduces maintainability

Summary

The GlobalConfiguration::removeCollateralFromLiquidationPriority function in the GlobalConfiguration library is implemented inefficiently. The current implementation involves making an in-memory copy of the entire set, erasing the entire set, and then rebuilding it without the target element. This approach results in higher gas costs and reduced maintainability due to the complexity of the loop logic.

Vulnerability Details

The difference in gas costs is even greater when the element to be removed is at the end of the array. This is due to the reduced number of swaps and pops required to remove elements from the array.

Let’s compare the gas costs of the current implementation with those of the proposed optimized function. Both implementations will use a structure similar to the one used in GlobalConfiguration.sol to provide the best approximation of gas costs:

  • Add the following to the test/unit/perpetuals/global-configuration/removeCollateralFromLiquidationPriority/removeCollateralFromLiquidationPriority.t.sol file:

import { EnumerableSet } from "@openzeppelin/utils/structs/EnumerableSet.sol";
import { Errors } from "@zaros/utils/Errors.sol";
import { console } from "forge-std/Test.sol";
// ...
library CurrentRemoveCollateral {
using EnumerableSet for *;
struct Data {
EnumerableSet.AddressSet collateralLiquidationPriority;
}
bytes32 internal constant GLOBAL_CONFIGURATION_LOCATION = keccak256(
abi.encode(uint256(keccak256("fi.zaros.perpetuals.GlobalConfiguration")) - 1)
) & ~bytes32(uint256(0xff));
function load() internal pure returns (Data storage config) {
bytes32 slot = GLOBAL_CONFIGURATION_LOCATION;
assembly {
config.slot := slot
}
}
function configureCollateralLiquidationPriority(Data storage self, address[] memory collateralTypes) internal {
uint256 cachedCollateralTypesCached = collateralTypes.length;
for (uint256 i; i < cachedCollateralTypesCached; i++) {
if (collateralTypes[i] == address(0)) {
revert Errors.ZeroInput("collateralType");
}
if (!self.collateralLiquidationPriority.add(collateralTypes[i])) {
revert Errors.MarginCollateralAlreadyInPriority(collateralTypes[i]);
}
}
}
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();
// 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]);
}
}
}
contract CurrentImplementation {
using CurrentRemoveCollateral for CurrentRemoveCollateral.Data;
using EnumerableSet for EnumerableSet.AddressSet;
function getCollateralLiquidationPriority() external view returns (address[] memory) {
CurrentRemoveCollateral.Data storage self = CurrentRemoveCollateral.load();
uint256 length = self.collateralLiquidationPriority.length();
address[] memory collateralLiquidationPriority = new address[](length);
for (uint256 i; i < length; i++) {
collateralLiquidationPriority[i] = self.collateralLiquidationPriority.at(i);
}
return collateralLiquidationPriority;
}
function configureCollateralLiquidationPriority(address[] memory collateralTypes) external {
CurrentRemoveCollateral.Data storage data = CurrentRemoveCollateral.load();
data.configureCollateralLiquidationPriority(collateralTypes);
}
function removeCollateralFromLiquidationPriority(address collateralType) external {
CurrentRemoveCollateral.Data storage data = CurrentRemoveCollateral.load();
data.removeCollateralFromLiquidationPriority(collateralType);
}
}
library ProposedRemoveCollateral {
using EnumerableSet for *;
struct Data {
EnumerableSet.AddressSet collateralLiquidationPriority;
}
bytes32 internal constant GLOBAL_CONFIGURATION_LOCATION = keccak256(
abi.encode(uint256(keccak256("fi.zaros.perpetuals.GlobalConfiguration")) - 1)
) & ~bytes32(uint256(0xff));
function load() internal pure returns (Data storage config) {
bytes32 slot = GLOBAL_CONFIGURATION_LOCATION;
assembly {
config.slot := slot
}
}
function configureCollateralLiquidationPriority(Data storage self, address[] memory collateralTypes) internal {
uint256 cachedCollateralTypesCached = collateralTypes.length;
for (uint256 i; i < cachedCollateralTypesCached; i++) {
if (collateralTypes[i] == address(0)) {
revert Errors.ZeroInput("collateralType");
}
if (!self.collateralLiquidationPriority.add(collateralTypes[i])) {
revert();
}
}
}
function removeCollateralFromLiquidationPriority(Data storage self, address collateralType) internal {
// Check if the collateral exists in the set
if (!self.collateralLiquidationPriority.contains(collateralType)) {
revert Errors.MarginCollateralTypeNotInPriority(collateralType);
}
// Get the current collateral order
address[] memory values = self.collateralLiquidationPriority.values();
uint256 length = values.length;
// Find the index of the collateral to remove
uint256 indexToRemove = type(uint256).max;
for (uint256 i = 0; i < length; i++) {
if (values[i] == collateralType) {
indexToRemove = i;
break;
}
}
// Remove the collateral from the set
if (indexToRemove < length) {
for (uint256 i = indexToRemove; i < length; i++) {
self.collateralLiquidationPriority.remove(values[i]);
}
for (uint256 i = indexToRemove + 1; i < length; i++) {
self.collateralLiquidationPriority.add(values[i]);
}
}
}
}
contract ProposedImplementation {
using ProposedRemoveCollateral for ProposedRemoveCollateral.Data;
using EnumerableSet for EnumerableSet.AddressSet;
function getCollateralLiquidationPriority() external view returns (address[] memory) {
ProposedRemoveCollateral.Data storage self = ProposedRemoveCollateral.load();
uint256 length = self.collateralLiquidationPriority.length();
address[] memory collateralLiquidationPriority = new address[](length);
for (uint256 i; i < length; i++) {
collateralLiquidationPriority[i] = self.collateralLiquidationPriority.at(i);
}
return collateralLiquidationPriority;
}
function configureCollateralLiquidationPriority(address[] memory collateralTypes) external {
ProposedRemoveCollateral.Data storage data = ProposedRemoveCollateral.load();
data.configureCollateralLiquidationPriority(collateralTypes);
}
function removeCollateralFromLiquidationPriority(address collateralType) external {
ProposedRemoveCollateral.Data storage data = ProposedRemoveCollateral.load();
data.removeCollateralFromLiquidationPriority(collateralType);
}
}
contract ImplementationGasTest is Base_Test {
CurrentImplementation currentImplementation = new CurrentImplementation();
ProposedImplementation proposedImplementation = new ProposedImplementation();
uint256 private constant NUMBER_OF_COLLATERAL = 100;
uint256 private constant COLLATERAL_INDEX_TO_REMOVE = 42;
address[] private collateralTypes = new address[](NUMBER_OF_COLLATERAL);
function setUp() public override {
for (uint256 i = 0; i < NUMBER_OF_COLLATERAL; i++) {
collateralTypes[i] = address(uint160(i + 1));
}
}
function test_CurrentVsProposedImplementation() external {
console.log("Element to remove: %s", collateralTypes[COLLATERAL_INDEX_TO_REMOVE]);
vm.startPrank({ msgSender: users.owner.account });
currentImplementation.configureCollateralLiquidationPriority(collateralTypes);
console.log(
"Number of collaterals configured before calling current implementation: %s",
currentImplementation.getCollateralLiquidationPriority().length
);
uint256 gasStart = gasleft();
currentImplementation.removeCollateralFromLiquidationPriority(collateralTypes[COLLATERAL_INDEX_TO_REMOVE]);
uint256 gasEnd = gasleft();
vm.stopPrank();
uint256 gasCurrentImplementation = gasStart - gasEnd;
console.log("Gas used for current Implementation:", gasCurrentImplementation);
console.log(
"Number of collaterals after calling current implementation: %s\n",
currentImplementation.getCollateralLiquidationPriority().length
);
vm.startPrank({ msgSender: users.owner.account });
proposedImplementation.configureCollateralLiquidationPriority(collateralTypes);
console.log(
"Number of collaterals configured before calling proposed implementation: %s",
proposedImplementation.getCollateralLiquidationPriority().length
);
gasStart = gasleft();
proposedImplementation.removeCollateralFromLiquidationPriority(collateralTypes[COLLATERAL_INDEX_TO_REMOVE]);
gasEnd = gasleft();
vm.stopPrank();
uint256 gasProposedImplementation = gasStart - gasEnd;
console.log("Gas used for proposed implementation:", gasProposedImplementation);
console.log(
"Number of collaterals after calling proposed implementation: %s\n",
proposedImplementation.getCollateralLiquidationPriority().length
);
console.log(
"Are the arrays the same? %s",
compareArrays(
currentImplementation.getCollateralLiquidationPriority(),
proposedImplementation.getCollateralLiquidationPriority()
)
);
}
function compareArrays(address[] memory array1, address[] memory array2) public pure returns (bool) {
if (array1.length != array2.length) {
return false;
}
for (uint256 i = 0; i < array1.length; i++) {
if (array1[i] != array2[i]) {
return false;
}
}
return true;
}
}

And run the test with the command forge test --mc ImplementationGasTest -vv. For a list of 100 elements, with a target element to remove at an arbitrary index 42, you should see a similar result:

[PASS] test_CurrentVsProposedImplementation() (gas: 13279012)
Logs:
Element to remove: 0x000000000000000000000000000000000000002b
Number of collaterals configured before calling current implementation: 100
Gas used for current implementation: 4288818
Number of collaterals after calling current implementation: 99
Number of collaterals configured before calling proposed implementation: 100
Gas used for proposed implementation: 2454619
Number of collaterals after calling proposed implementation: 99
Are the arrays the same? true
// A difference of 1,834,199 in gas cost.

Now, even if we define the COLLATERAL_INDEX_TO_REMOVE as the first element of the list at index 0, we get a slight improvement in terms of cost:

Logs:
Element to remove: 0x0000000000000000000000000000000000000001
Gas used for current Implementation: 4288811
Gas used for proposed implementation: 4257748
Are the arrays the same? true

If it's the last, at index 99:

Logs:
Element to remove: 0x0000000000000000000000000000000000000064
Gas used for current Implementation: 4288818
Gas used for proposed implementation: 34122
Are the arrays the same? true

Impact

  • Increased Gas Costs: The multiple storage operations required by the current implementation result in higher gas fees for the owners.

  • Reduced Maintainability: The complex logic makes the function harder to read, understand, and maintain, increasing the likelihood of errors during future modifications.

  • Potential Performance Bottleneck: Even though the set should contains less than 10 elements, the inefficiency could still be noticeable.

Tools Used

Manual review

Recommendations

Optimize the removeCollateralFromLiquidationPriority function by removing the target element and the elements after, while preserving the order of the remaining elements. Here is an improved version of the function:

function removeCollateralFromLiquidationPriority(Data storage self, address collateralType) internal {
// Check if the collateral exists in the set
if (!self.collateralLiquidationPriority.contains(collateralType)) {
revert Errors.MarginCollateralTypeNotInPriority(collateralType);
}
// Get the current collateral order
address[] memory values = self.collateralLiquidationPriority.values();
uint256 length = values.length;
// Find the index of the collateral to remove
uint256 indexToRemoveFrom = type(uint256).max;
for (uint256 i = 0; i < length; i++) {
if (values[i] == collateralType) {
indexToRemoveFrom = i;
break;
}
}
if (indexToRemoveFrom < length) {
// Remove the collateral from the set and the following elements
for (uint256 i = indexToRemoveFrom; i < length; i++) {
self.collateralLiquidationPriority.remove(values[i]);
}
// indexToRemoveFrom + 1 in order to skip the element to remove
for (uint256 i = indexToRemoveFrom + 1; i < length; i++) {
self.collateralLiquidationPriority.add(values[i]);
}
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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