Salty.IO - eta's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingConfig.sol#L341-L1006 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAOConfig.sol#L641-L19710 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolsConfig.sol#L771-L10810 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/RewardsConfig.sol#L362-L10010 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/StableConfig.sol#L472-L15210 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L651-L964 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolMath.sol#L2101-L2283

Vulnerability details

1、The range and adjustment in functions are inconsistent with their notices in configs’ contract.

Impact

Inconsistencies between the notices and actual code for range and adjustment functions in multiple contracts, including StakingConfig, DAOConfig, PoolsConfig, RewardsConfig, StableConfig, and PriceAggregator, are causing confusion and potential financial losses. This stems from unclear standards and variations in how ranges are adjusted, leading to discrepancies between expected and actual outcomes. Manual calculations are often required, hindering user experience and increasing the risk of errors. To address this, a clear, unified standard for range and adjustment is urgently needed in both code and whitebook, with consistent implementation across all affected contracts. To remain the standard of Range: x to x with an adjustment of x, that is, adjusting the range with each adjustment iteration, including raising the upper limit and lowering the lower limit, one of the simplest approach is to replace > and < with >= and <=, respectively. In cases where 0 might be involved, adding +1 to the lower limit parameters can address this issue.

Proof of Concept

In the StakingConfig:: changeMaxUnstakeWeeks, the notice of Range: 20 to 108 with an adjustment of 8 implies that the range for maxUnstakeWeeks is from 20 to 80, with an adjustment of 8, and the upper limit can be adjusted to +8, resulting in 116, and the lower limit can be adjusted to -8, resulting in 22. However, based on the conditional statements in the function, if (maxUnstakeWeeks < 108) maxUnstakeWeeks += 8, and if (maxUnstakeWeeks > 20) maxUnstakeWeeks -= 1, the actual upper limit for maxUnstakeWeeks can be adjusted to 107 + 8 = 115, and the actual lower limit can be adjusted to 21 - 8 = 13. Consequently, the actual adjusted range in this function is from 13 to 115, but the notice indicates a adjusted range of 12 to 116, which means the difference of upper limit is 115 - 116 = -1 while the difference of lower limit is 13 - 12 = +1, making the standard of range and adjustment inconsistent and confused.

Similar inconsistencies exist in the changeMinUnstakePercent and changeModificationCooldown functions of the contract. The notice for these functions suggests adjusted ranges of 5 to 55, 6 hours 15 minutes to 0 minutes, respectively. However, the actual adjusted ranges in the functions are 6 to 54, 6 hours 14 minutes to 1 minute, respectively.

Furthermore, in the changeMinUnstakeWeeks function, there is a case where the notice's adjusted range matches the original range: the notice range is 1 to 12, and the adjusted range after modification is 0 to 13. However, the function' adjusted range remains 1 to 12, similar to its notice unadjusted range, according to if (minUnstakeWeeks < 12) minUnstakeWeeks += 1 and if (minUnstakeWeeks > 1) minUnstakeWeeks -= 1. This may lead users to misunderstand that the standard is that within the range, there is adjustment, rather than the standard of adjusting the range with each adjustment iteration, including raising the upper limit and lowering the lower limit.

Such inconsistencies between the notice and functions in range and adjustment can lead to confusion. Users may be uncertain about the standards for range and adjustment—whether it is the the standard A, for each adjustment within the range, the upper limit can be increased and the lower limit can be decreased, or whether it is the standard B, within the range, each adjustment is made. Even with the standard A, it is not intuitive and has variations, requiring manual calculations each time. Additionally, such inconsistencies are present in other contracts related to configuration, including DAOConfig.sol, PoolsConfig.sol, RewardsConfig.sol, StableConfig.sol, and PriceAggregator.sol. If calculations are required each time, it can negatively impact the user experience that the calculation errors mentioned can potentially lead to financial losses.

DAOConfig PoolsConfig RewardsConfig StableConfig PriceAggregator

Therefore, in both the code and the whitepaper's parameters section, a clear, simple and unified standard for range and adjustment is needed. This would facilitate users' understanding and usage of these parameters, ultimately enhancing the user experience and finance. To remain the standard A of adjusting the range with each adjustment iteration, including raising the upper limit and lowering the lower limit, one of the simplest approach is to replace > and < with >= and <=, respectively. In cases where 0 might be involved, adding +1 to the lower limit parameters can address this issue. This adjustment can be applied consistently across all the six contracts, as shown in the example of StakingConfig contract below:

Whitepaper: Parameters

StakingConfig::changeMinUnstakeWeeks, changeMaxUnstakeWeeks, changeMinUnstakePercent, changeModificationCooldown

    // Range: 2 to 12 with an adjustment of 1
    function changeMinUnstakeWeeks(bool increase) external onlyOwner {
        if (increase) {
-           if (minUnstakeWeeks < 12) minUnstakeWeeks += 1;
+           if (minUnstakeWeeks <= 12) minUnstakeWeeks += 1;
        } else {
-           if (minUnstakeWeeks > 1) minUnstakeWeeks -= 1;
            // @audit: To prevent the occurrence of 0 values, add 1 to the lower limit.
+           if (minUnstakeWeeks >= 2) minUnstakeWeeks -= 1;

        }

        emit MinUnstakeWeeksChanged(minUnstakeWeeks);
}

   // Range: 20 to 108 with an adjustment of 8
    function changeMaxUnstakeWeeks(bool increase) external onlyOwner {
        if (increase) {
-           if (maxUnstakeWeeks < 108) maxUnstakeWeeks += 8;
+           if (maxUnstakeWeeks <= 108) maxUnstakeWeeks += 8;
        } else {
-           if (maxUnstakeWeeks > 20) maxUnstakeWeeks -= 8;
+           if (maxUnstakeWeeks >= 20) maxUnstakeWeeks -= 8;
        }

        emit MaxUnstakeWeeksChanged(maxUnstakeWeeks);
    }

// Range: 10 to 50 with an adjustment of 5
    function changeMinUnstakePercent(bool increase) external onlyOwner {
        if (increase) {
-           if (minUnstakePercent < 50) minUnstakePercent += 5;
+           if (minUnstakePercent <= 50) minUnstakePercent += 5;
        } else {
-           if (minUnstakePercent > 10) minUnstakePercent -= 5;
+           if (minUnstakePercent >= 10) minUnstakePercent -= 5;
        }

        emit MinUnstakePercentChanged(minUnstakePercent);
    }
// Range: 16 minutes to 6 hours with an adjustment of 15 minutes
    function changeModificationCooldown(bool increase) external onlyOwner {
        if (increase) {
-           if (modificationCooldown < 6 hours) modificationCooldown += 15 minutes;
+          if (modificationCooldown <= 6 hours) modificationCooldown += 15 minutes;
        } else {
-         if (modificationCooldown > 15 minutes) modificationCooldown -= 15 minutes;
            // @audit To prevent the occurrence of 0 values, add 1 to the lower limit.
+       if (modificationCooldown >= 16 minutes) modificationCooldown -= 15 minutes;
        }

        emit ModificationCooldownChanged(modificationCooldown);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

To remain the standard of Range: x to x with an adjustment of x, that is, adjusting the range with each adjustment iteration, including raising the upper limit and lowering the lower limit, one of the simplest approach is to replace > and < with >= and <=, respectively. In cases where 0 might be involved, adding +1 to the lower limit parameters can address this issue as shown in Proof of Concept.

2. Omitting the equal condition in the PoolMath:: _determineZapSwapAmount

Impact

Omitting the condition zapAmountA * reserveB == reserveA * zapAmountB in the _determineZapSwapAmount function would result in incorrect swap calculations when the ratios of zapAmountA/zapAmountB and reserveA/reserveB are equal.

Proof of Concept

if the condition zapAmountA * reserveB == reserveA * zapAmountB was not considered in the _determineZapSwapAmount function in the PoolMath contract.

Without this condition, the function would fail to handle cases where the condition zapAmountA * reserveB and reserveA * zapAmountB are exactly equal, which would lead to incorrect swap amounts.

The function's logic relies on the two if statements to determine which asset to swap based on the condition. However, if it is equal, neither if condition would be met, and the function would likely return incorrect swap amounts, potentially leading to unintended financial consequences.

Here's a recommendation to change the code to address the missing condition

PoolMath::_determineZapSwapAmount

	function _determineZapSwapAmount( uint256 reserveA, uint256 reserveB, uint256 zapAmountA, uint256 zapAmountB ) internal pure returns (uint256 swapAmountA, uint256 swapAmountB )
		{
		// zapAmountA / zapAmountB exceeds the ratio of reserveA / reserveB? - meaning too much zapAmountA
-		if ( zapAmountA * reserveB > reserveA * zapAmountB )
+		if ( zapAmountA * reserveB >= reserveA * zapAmountB )
			(swapAmountA, swapAmountB) = (_zapSwapAmount( reserveA, reserveB, zapAmountA, zapAmountB ), 0);

		// zapAmountA / zapAmountB is less than the ratio of reserveA / reserveB? - meaning too much zapAmountB
		if ( zapAmountA * reserveB < reserveA * zapAmountB )
			(swapAmountA, swapAmountB) = (0, _zapSwapAmount( reserveB, reserveA, zapAmountB, zapAmountA ));

		// Ensure we are not swapping more than was specified for zapping
		if ( ( swapAmountA > zapAmountA ) || ( swapAmountB > zapAmountB ) )
			return (0, 0);

		return (swapAmountA, swapAmountB);
		}
	}

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to add the equal condition to the if statement as shown in the Proof of Concept:

-		if ( zapAmountA * reserveB > reserveA * zapAmountB )
+		if ( zapAmountA * reserveB >= reserveA * zapAmountB )

Assessed type

Math