Owner can add more tokens than `MAX_TOKENS` in `BasketFacet`

[code423n4]

Handle

Czar102

Vulnerability details

Impact

In BasketFacet::addToken(...), the MAX_TOKENS check is done before possible reentrancy in the balance(...) function, which is done before adding the token to bs.

An owner can pass ownership to the contract, which adds contracts as tokens, whose balanceOf(...) function allows the attacker contract to call addToken(...) again. The MAX_TOKENS check then passes, because the first added token hasn't been added to bs yet. Thus, an attacker owner can add any number of tokens, being constrained only by block gas limit and stack depth.

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider making the reentrant call a part of the first check in the function.

[0xleastwood]

There is no attack vector. The protectedCall modifier is restricted to a defaultController account. Even then, there is nothing that can be done by reentering the function.

Additionally, I'm not sure if reentrancy is fully understood here. An external call is made to _token, which has to be a contract that the owner controls. There is no benefit by adding useless tokens, except to deny other tokens from being added. But they are the only ones allowed to add tokens, so they are DOS'ing themselves.

Marking as non-critical as it is good practice to implement the checks-effects pattern.