DittoETH

Summary

The updateErcDebt function assumes that the shortId provided is valid and that a short record exists for the given asset and shorter. If this is not the case, the function could potentially behave unexpectedly.

Vulnerability Details

The updateErcDebt() function is designed to update the ercDebt of a specific short record. It retrieves a reference to the specific short record from the shortRecords mapping using the provided asset, shorter, and shortId.

If the shortId does not correspond to an existing short record, the function attempts to access and modify a non-existent record in the shortRecords mapping.

This does not result in an error, but instead returns a struct with all fields set to their default values. The function then attempts to calculate ercDebt using these default values, which would likely result in ercDebt being 0. The subsequent check if (ercDebt > 0) would then fail, and the function would end without modifying any state.

Impact

This logical error could potentially lead to incorrect behavior in functions that call updateErcDebt(), such as the decreaseCollateral() function. If updateErcDebt() does not update the ercDebt as expected due to an invalid shortId, the subsequent calculations and checks in decreaseCollateral() that rely on the ercDebt could be incorrect. This includes the calculation of the collateral ratio and the check if the new collateral is above the minimum required margin. This could potentially lead to financial loss or other adverse effects.

updateErcDebt() does not correctly update the ercDebt due to an invalid shortId, the calculations and checks in increaseCollateral() and decreaseCollateral() functions, which both call updateErcDebt() could be incorrect.

Tools Used

Manual Review

Recommendations

To mitigate this issue, it is recommended to include a check at the start of the updateErcDebt() function to ensure that the shortId provided corresponds to an existing short record. If it does not, the function could revert with an appropriate error message. This would make the function's behavior more predictable and easier to reason about. As always, it's recommended to have any smart contract code thoroughly audited by a professional security firm before deployment.

in Solidity, there's no direct way to check if a specific key exists in a mapping.

One way to handle this is to add an additional data structure, such as an array or a mapping, to keep track of the existing shortIds. You could then check this data structure before attempting to update the ercDebt.

Here's an example of how you could modify the updateErcDebt() function:

function updateErcDebt(address asset, address shorter, uint8 shortId) internal {
AppStorage storage s = appStorage();

// Check if the shortId exists
require(s.shortIdExists[asset][shorter][shortId], "Invalid shortId");

STypes.ShortRecord storage short = s.shortRecords[asset][shorter][shortId];

// 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;
}

}
In this example, shortIdExists is a new mapping that you would need to maintain whenever a new short record is created or deleted. The require statement ensures that the function reverts if the shortId does not exist, preventing the function from operating on non-existent records.