removeAddress doesn't decrease the contracts.length

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L93

Vulnerability details

Impact

The contracts length will always increase because the removeAddress() function just deleting the value inside the array and never decrease the length by calling pop() method. This can lead to Dos when calling functions that doing loop on contracts storage: https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L16 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L36 https://github.com/code-423n4/2022-06-yieldy/blob/main/src/contracts/BatchRequests.sol#L91

Recommended Mitigation Steps

Implement the pop() method for dynamic array so we can avoid unnecessary storage reading for zero value in the future

[Picodes]

As addAddress is only owner, this is a low issue at best as the array length cannot be arbitrarily increased

[toshiSat]

disagree with severity: low severity due to onlyOwner function.

[0x1f8b]

@toshiSat @Picodes It's onlyOwner method but the logic is wrong, so you don't need to attack nothing or have a bad actor here, if the owner try to remove an address, the service will be denied, that's cannot be low...

[Picodes]

@0x1f8b indeed, but it seems in the important function of the contract it won't revert due to the test contracts[i] != address(0)

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L18

[0x1f8b]

@0x1f8b indeed, but it seems in the important function of the contract it won't revert due to the test contracts[i] != address(0)

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L18

Not in https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L37