The function `addToken` does not check if the token was already added

[code423n4]

Handle

hrkrshnn

Vulnerability details

addToken does not check if the token was already added

The function addToken does not check if the token was already present.

function addToken(
    address _vault,
    address _token
)
    external
    override
    notHalted
    onlyStrategist
{
    require(allowedTokens[_token], "!allowedTokens");
    require(allowedVaults[_vault], "!allowedVaults");
    require(tokens[_vault].length < MAX_TOKENS, ">tokens");
    vaults[_token] = _vault;
    tokens[_vault].push(_token);
    emit TokenAdded(_vault, _token);
}  

Double counting issue with balanceOfThis and therefore withdraw

In balanceOfThis, if a token is present in the vault tokens list twice, it's balance is counted twice, leading to double counting in the balanceOfThis computation

function balanceOfThis()
    public
    view
    returns (uint256 _balance)
{
    address[] memory _tokens = manager.getTokens(address(this));
    for (uint8 i; i < _tokens.length; i++) {
        address _token = _tokens[i];
        _balance = _balance.add(_normalizeDecimals(_token, IERC20(_token).balanceOf(address(this))));
    }
}  

Since withdraw function uses balanceOfThis to compute the amount to be sent back, this will lead to user withdrawing more money than they should have.

Issue with removeToken

The problem is that the function removeToken seem to assume that all tokens are only present once in the dynamic array. This means that if you add the same token twice, the call to removeToken only removes it once. This could potentially create issues.

Recommended Mitigation Steps

Unfortunately, checking this on chain with the current dynamic array architecture will be expensive. It is recommended to use a mapping or an enumerable set / mapping instead. The following is a sample implementation.

modified   contracts/v3/Manager.sol
@@ -59,7 +59,8 @@ contract Manager is IManager {
     // vault => controller
     mapping(address => address) public override controllers;
     // vault => tokens[]
-    mapping(address => address[]) public override tokens;
+    mapping(address => mapping(address => bool)) public override tokens;
+    uint numTokens = 0;
     // token => vault
     mapping(address => address) public override vaults;

[Haz077]

Fixed in code-423n4/2021-09-yaxis#2

[uN2RVw5q]

Duplicate of https://github.com/code-423n4/2021-09-yaxis-findings/issues/3

[GalloDaSballo]

Duplicate of #3