ravikiran.web3
medium
The logic implemented for swapCollateralIndexes is not defensive enough. This could lead to dangerous update on state variables. The index variables passed to the function should be less than the size of the collateral array.
The only line of the defence is the modifier attached that restricts who can call this function.
In the swapCollateralIndexes function, the attacker could pass index1 as uint256.max number. This will result in copying of invalid value at uint256.max into the index2. And value in index2 copied over to position uint256.max in the array. This will result in loss of collateral record at index 1.
The array is now initiaized to uint256.max length resulting in DOS, and there is no way to correct this at later point.
Loss of collatera value Initialization of array to max size resulting in DOS due to lack of gas to support it.
https://github.com/sherlock-audit/2023-05-USSD/blob/main/ussd-contracts/contracts/USSD.sol#L110-L118
function swapCollateralIndexes( uint256 _index1, uint256 _index2 ) public onlyControl { // cannot use (a, b) = (b, a) for storage variables CollateralInfo memory tmp = collateral[_index1]; collateral[_index1] = collateral[_index2]; collateral[_index2] = tmp; }
Manual Review
Check for both _index1 and _index2 to be lass than collateral length before performing a swap.