code-423n4 / 2023-01-astaria-findings

5 stars 2 forks source link

INCORRECT FUNCTION CALLS #561

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L28-L36 https://github.com/code-423n4/2023-01-astaria/blob/main/src/WithdrawProxy.sol#L103-L111 https://github.com/code-423n4/2023-01-astaria/blob/main/src/PublicVault.sol#L76-L84

Vulnerability details

Impact

In Vault.sol, WithdrawProxy.sol and PublicVault.sol, name() is making an incorrect external call to return its output variable. Apparently, it is calling ERC20(asset()).symbol()) instead of ERC20(asset()).name()). Devoid of an accurate name description, this could lead to confusion and perhaps trick users/developers into mistaking it for another asset.

Proof of Concept

The following instances show the wrong inclusion of the second variable in string(abi.encodePacked():

Vault.sol#L28-L36

  function name()
    public
    view
    virtual
    override(VaultImplementation)
    returns (string memory)
  {
    return string(abi.encodePacked("AST-Vault-", ERC20(asset()).symbol()));
  }

WithdrawProxy.sol#L103-L111

  function name()
    public
    view
    override(IERC20Metadata, WithdrawVaultBase)
    returns (string memory)
  {
    return
      string(abi.encodePacked("AST-WithdrawVault-", ERC20(asset()).symbol()));
  }

PublicVault.sol#L76-L84

  function name()
    public
    view
    virtual
    override(IERC20Metadata, VaultImplementation)
    returns (string memory)
  {
    return string(abi.encodePacked("AST-Vault-", ERC20(asset()).symbol()));
  }

Recommended Mitigation Steps

It is recommended replacing ERC20(asset()).symbol()) with ERC20(asset()).name()) in the return statement to clear up the errors.

Picodes commented 1 year ago

What is the issue if the name of the vault is defined as "AST-Vault-" + the underlying token symbol? This is just a matter of convention, especially as names aren't suppose to be unique or anything.

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid