The function `removeToken` can get prohibitively expensive

[code423n4]

Handle

hrkrshnn

Vulnerability details

The function removeToken can get prohibitively expensive

The function removeToken loops over a dynamic array, and compares values. This can quickly get expensive as more and more tokens get added. The for loop has two sload operations, assuming cold costs for both these sload, since the maximum number of allowed tokens is 256, the storage costs alone can cost around 1 million gas (2 * 2100 * 256 = 1075200) This is an unsustainable amount of gas for a protocol operation and therefore adding this as a low severity bug instead of as a gas optimization.

One way to prevent this issue is to compute the index off chain and pass it as an argument on chain.

For example:

function removeToken(address _vault, address _token, uint index) public {
    uint256 k = tokens[_vault].length;
    uint256 index;
    bool found;
    require(tokens[_vault][index] == _token);
    // do the removal
}

Alternatively, changing the dynamic array to a mapping can also be considered.

[Haz077]

I agree it can get gas-expensive but in my opinion index shouldn't be computed off chain.

[GalloDaSballo]

Agree with finding (gas optimization) Solution suggested by other devs is to use EnumerableSet