`_setAmplificationData` should clear upper bits of values

[code423n4]

Handle

cmichel

Vulnerability details

The TempusAMM._setAmplificationData function is supposed to pack four 64 bit values by shifting and oring them. However, note that these values are 256-bit values (uint256) and might contain set bits outside the 64 lower bits.

Impact

When computing the or, these bits will influence the other values which leads to undesired results when decoding the amplification data. For example, a 256-bit endTime can be chosen in startAmplificationParameterUpdate which then influences the other values.

Recommended Mitigation Steps

The _setAmplificationData function should first truncate the four parameters to 64-bits, for example by changing the function signature to take uint64s instead of uint256s.

[mijovic]

This method is not publically exposed. When called in the contract(only in one place where it has external arbitrary args) there is a check for min and max, so it clearly fits uint64. This shouldn't be considered as issue

Defined in StableMath.sol from upstream:

uint256 internal constant _MIN_AMP = 1;
uint256 internal constant _MAX_AMP = 5000;

And here you can find that it is being checked in TempusAMM.sol: https://github.com/code-423n4/2021-10-tempus/blob/main/contracts/amm/TempusAMM.sol#L122 https://github.com/code-423n4/2021-10-tempus/blob/main/contracts/amm/TempusAMM.sol#L718

[0xean]

Downgrading to 0 severity based on the current usage there doesn't seem to be an issue with the intended functionality. However modifying the function signature may allow for better code clarity if these contracts are ever extended to future use.