Summary

In the function removeBeneficiary(address _beneficiary) the owner get the index of the beneficiary from the function call uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);. So , it should return the valid index number in order to delete the beneficiary. But it is not. Also the low level function(delete) is increasing the gas costs by keeping the storage blank.

Vulnerability Details

In the below code if you see, what happen if the benficiary is not found in the code, the _index return value which is at default have value of 0 is passed to the line of code uint256 indexToRemove = _getBeneficiaryIndex(_beneficiary);resulting in deletion of beneficiary at index delete beneficiaries[0].

Impact

1 . Every time function is called by the owner the benficiary at index 0 gets deleted. If the benficiary does not exists.

2.This result in blank slot in the array . The array still has the same length, but index 0 is now address(0), which is useless.

3.The array does not shrink, so unnecessary storage costs are maintained. This increases gas costs because unnecessary reads and writes are performed.

Tools Used

Mannual review

Recommendations

1.Always use pop() when removing array elements.** In the long run** delete does not reduce the array length, meaning future operations still process unnecessary storage.

// Move last element to the deleted position
beneficiaries[index] = beneficiaries[beneficiaries.length - 1];

// Remove the last element````

beneficiaries.pop();

1. low-level function (revert) used: