eta - Forcing conversion of _cvgControlTower.cvgCycle() can cause accounting errors in missing values

[sherlock-admin2]

eta

medium

Forcing conversion of _cvgControlTower.cvgCycle() can cause accounting errors in missing values

Summary

The current protocol CvgControlTower.sol uses uint128 for cvgCycle, while LockingPositionService.sol uses a mix of uint96, uint128, and uint256, which can lead to overflows during type conversions. SafeCast library provides safe type conversion functions to prevent overflows and ensure contract security.

Vulnerability Detail

The current protocol CvgControlTower.sol has uint128 for cvgCycle, but all of uint96, uint128, uint256 are used in LockingPositionService.sol. So in lockingInfo /increaseLockTime /increaseLockTimeAndAmount /burnPosition /updateYsTotalSupply /tokenInfos /balanceOfMgCvg /totalSupplyOfYsCvgAt, it is forced to convert to different units.

Casting from larger data types to smaller ones can result in overflows, leading to unexpected behavior. When the uint128 value gets cast to uint96, it can be transformed into a significantly smaller number due to the reduction in bits.

OpenZeppelin's SafeCast library offers functions for safe type conversions, raising an error when an overflow occurs. It is generally advisable to employ SafeCast or similar safeguards during type conversions to guarantee the integrity of calculations and contract security.

2 issue instances in 1 files: File: Locking/LockingPositionService.sol

1)LockingPositionService.sol#L249C6-L250C1

Suggestions:

uint96 actualCycle = SafeCast.toUint96(_cvgControlTower.cvgCycle());

2)LockingPositionService.sol#L249C6-L250C1

The function retrieves the actual cycle using _cvgControlTower.cvgCycle(), which returns a uint128. However, later in the function, the newEndCycle - actualCycle - 1 value is compared to MAX_LOCK, which is a uint96. This comparison can potentially lead to an overflow and unexpected behavior.

To illustrate this, consider the following scenario:

uint128 maxLock = uint128(MAX_LOCK); // Max lock is 96 cycles
uint128 actualCycle = 95; // Current cycle is 95
uint128 newEndCycle = actualCycle + 1; // New lock end cycle is 96

require(newEndCycle - actualCycle - 1 <= maxLock, "MAX_LOCK_96_CYCLES");

In this scenario, newEndCycle is also equal to 96. When the comparison is performed, uint128(newEndCycle - actualCycle - 1) is implicitly converted to a uint96. However, uint96(96) - uint96(95) - uint96(1) results in an overflow, causing the comparison to evaluate to false.

This could lead to unexpected behavior, such as failing to increase the lock time as intended.

Suggestion:

SafeCast.toUint96(MAX_LOCK)

Impact

1) Unexpected behavior: The function may fail to increase the lock time as intended, leading to unexpected consequences for the user. 2) Security vulnerability: If the overflow is not detected, it could potentially be exploited by malicious actors to manipulate the function's behavior.

Code Snippet

1)LockingPositionService.sol#L249C6-L250C1 2)LockingPositionService.sol#L249C6-L250C1

Tool used

Manual Review

Recommendation

It is recommended use SafeCast.toUint96() to ensure that the comparison is performed safely, as it explicitly checks for overflows and protects against potential security vulnerabilities.

[nevillehuang]

Invalid, there is some misconception here. The maximum value cygCycle can take is 2**128 - 1.

While it is true that cygCycle is declared as a uint128, given each cycle is 7 days, it would take (2128 - 1) cycles for it to overflow, meaning (2128 - 1) * 7 days = ~2.38e39 days for this to overflow, that is ~6.53e36 years, which is virtually impossible.