SetSymbol function can change during the lifetime of the contract implementation.

Summary

The ERC20Ubiquity contract allows a trusted admin to re-set the token symbol multiple times, which can produce unexpected behavior in other contract implementations.

Vulnerability Detail

The token contract adheres to the ERC20 standard implementation. It is unlikely that users will expect certain standard values, such as the token symbol, to change over time, as that possibility is not mentioned in the ERC20 standard.

Impact

If a trusted admin or governance process allows to update these values during the lifetime of the contract implementation or if they are changed due to some operational reasons, it can lead to unexpected results.

For instance, when this token contract is initialized with a token name and a symbol, these identifiers are to be saved as state variables. After that, they are not expected to change.

Code Snippet

     /**
     * @notice Updates token symbol
     * @param newSymbol New token symbol name
     */
    function setSymbol(string memory newSymbol) external onlyAdmin {
        _symbol = newSymbol;
    }

    /**
     * @notice Returns token symbol name
     * @return Token symbol name
     */
    function symbol() public view virtual override returns (string memory) {
        return _symbol;
    }

Tool used

Manual Review

Recommendation

One should expect the below process flow to fail, but the condition gets passed without any issue. I think it is necessary to develop clear documentation for users and other associated parties on this unexpected property.

      function testSetSymbol_ShouldSetSymbolTwice() public {
        vm.prank(admin);
        dollarToken.setSymbol("ANY_SYMBOL");
        assertEq(dollarToken.symbol(), "ANY_SYMBOL");

        vm.prank(admin);
        dollarToken.setSymbol("ANY_SYMBOL_2");
        assertEq(dollarToken.symbol(), "ANY_SYMBOL_2");
    }

In the long term, the team should refer to this Token Integration Checklist and implement its recommendations to make sure that deployed ERC20 tokens behave as they are expected to do so.

sherlock-admin2 commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

It's protocol decision