token -> vault mapping can be overwritten

[code423n4]

Handle

cmichel

Vulnerability details

One vault can have many tokens, but each token should only be assigned to a single vault. The Manager contract keeps a mapping of tokens to vaults in the vaults[_token] => _vault map, and a mapping of vault to tokens in tokens[vault] => token[].

The addToken function can assign any token to a single vault and allows overwriting an existing vaults[_token] map entry with a different vault. This indirectly disassociates the previous vault for the token. Note that the previous vault's tokens[_previousVault] map still contains the token.

Impact

The token disappears from the system for the previous vault but the actual tokens are still in there, getting stuck. Only the new vault is considered for the token anymore, which leads to many issues, see Controller.getBestStrategyWithdraw and the onlyVault modifier that doesn't work correctly anymore.

Recommended Mitigation Steps

It should check if the token is already used in a map, and either revert or correctly remove the token from the vault - from the tokens array. It should do the same cleanup procedure as in removeToken:

if (found) {
    // remove the token from the vault
    tokens[_vault][index] = tokens[_vault][k-1];
    tokens[_vault].pop();
    delete vaults[_token];
    emit TokenRemoved(_vault, _token);
}

addToken should also check that the token is not already in the tokens[_vault] array.

[GalloDaSballo]

Mapping mismatch can cause undefined behaviour

Recommend having one source of truth to keep things simple