Astaria contest - btk's results

Total L issues

Total NC issues

[L-01] Use safeMint instead of mint for ERC721

Impact

Users could lost their NFTs if msg.sender is a contract address that does not support ERC721, the NFT can be frozen in the contract forever.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

As per the documentation of ERC721.sol by Openzeppelin:

Ref: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L274-L285

Lines of code

• CollateralToken.sol:588
• LienToken.sol:455

Recommended Mitigation Steps

Use _safeMint instead of mint to check received address support for ERC721 implementation.

    function _safeMint(
        address to,
        uint256 tokenId,
        bytes memory data
    ) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }

[L-02] ERC4626 does not work with fee-on-transfer tokens

Impact

The ERC4626-Cloned.deposit/mint functions do not work well with fee-on-transfer tokens as the assets variable is the pre-fee amount, including the fee, whereas the totalAssets do not include the fee anymore.

This can be abused to mint more shares than desired.

  function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    require(shares > minDepositAmount(), "VALUE_TOO_SMALL");
    // Need to transfer before minting or ERC777s could reenter.
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    emit Deposit(msg.sender, receiver, assets, shares);

    afterDeposit(assets, shares);
  }

Lines of code

• ERC4626-Cloned.sol:19

Recommended Mitigation Steps

assets should be the amount excluding the fee (i.e the amount the contract actually received), therefore it's recommended to use the balance change before and after the transfer instead of the amount.

[L-03] ERC4626Cloned 's implmentation is not fully up to EIP-4626's specification

Must return the maximum amount of shares mint would allow to be deposited to receiver and not cause a revert, which must not be higher than the actual maximum that would be accepted (it should underestimate if necessary).

This assumes that the user has infinite assets, i.e. must not rely on balanceOf of asset.

  function maxDeposit(address) public view virtual returns (uint256) {
    return type(uint256).max;
  }

  function maxMint(address) public view virtual returns (uint256) {
    return type(uint256).max;
  }

Could cause unexpected behavior in the future due to non-compliance with EIP-4626 standard.

Lines of code

• ERC4626-Cloned.sol:147

Recommended Mitigation Steps

maxMint() and maxDeposit() should reflect the limitation of maxSupply.

[L-04] Lack of nonReentrant modifier

It is a best practice to use the nonReentrant modifier when you make external calls to untrustable entities because even if an auditor did not think of a way to exploit it, an attacker just might.

Lines of code

• CollateralToken.sol:578
• AstariaRouter.sol:L519

[L-05] No storage gap for upgradeable contracts

Impact

For upgradeable contracts, inheriting contracts may introduce new variables. In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable contract.

In order to be able to add new variables to the upgradeable contract without causing storage collisions, a storage gap should be added to the upgradeable contract.

See this for a description of this storage variable.

Lines of code

• CollateralToken.sol
• AstariaRouter.sol
• LienToken.sol

Recommended Mitigation Steps

Consider adding a storage gap at the end of the upgradeable contract:

  /**
   * @dev This empty reserved space is put in place to allow future versions to add new
   * variables without shifting down storage in the inheritance chain.
   * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
   */
  uint256[50] private __gap;

[L-06] Take warnings seriously

If the compiler warns you about something, you should change it. Even if you do not think that this particular warning has security implications, there might be another issue buried beneath it. Any compiler warning we issue can be silenced by slight changes to the code.

Ref: https://docs.soliditylang.org/en/v0.8.17/security-considerations.html#take-warnings-seriously

Lines of code

• ClearingHouse.sol
• CollateralToken.sol
• PublicVault.sol
• AstariaRouter.so
• LienToken.sol

[L-07] Solmate's SafeTransferLib doesn't check whether the ERC20 contract exists

Solmate's SafeTransferLib.sol, which is often used to interact with non-compliant/unsafe ERC20 tokens, does not check whether the ERC20 contract exists. The following code will not revert in case the token doesn't exist.

Ref: https://github.com/transmissions11/solmate/blob/main/src/utils/SafeTransferLib.sol#L9

Lines of code

• AstariaRouter.sol:19
• ClearingHouse.sol:19
• CollateralToken.sol:32
• LienToken.sol:36
• PublicVault.sol:19
• Vault.sol:17
• VaultImplementation.sol:19
• WithdrawProxy.sol:18

Recommended Mitigation Steps

Add a contract exist check in functions or use Openzeppelin safeERC20 instead.

[NC-01] Lack of event emit

The below methods do not emit an event when the state changes, something that it's very important for dApps and users.

Lines of code

• AstariaRouter.sol:352
• PublicVault.sol:507
• LienToken.sol#:122
• LienToken.sol:233
• LienToken.sol:292
• LienToken.sol:512
• LienToken.sol:690

[NC-02] Require messages are too short and unclear

The correct and clear error description explains to the user why the function reverts, but the error descriptions below in the project are not self-explanatory. These error descriptions are very important in the debug features of DApps like Tenderly. Error definitions should be added to the require block, not exceeding 32 bytes.

Lines of code

• Vault.sol:65
• Vault.sol:71
• ClearingHouse.sol:72
• ClearingHouse.sol:199
• ClearingHouse.sol:216
• ClearingHouse.sol:223
• CollateralToken.sol:266
• CollateralToken.sol:535
• CollateralToken.sol:564
• PublicVault.sol:241
• PublicVault.sol:259
• PublicVault.sol:508
• PublicVault.sol:67
• PublicVault.sol:680
• PublicVault.sol:687
• LienToken.sol:504
• LienToken.sol:860
• VaultImplementation.sol:78
• VaultImplementation.sol:105
• VaultImplementation.sol:147
• VaultImplementation.sol:191
• AstariaRouter.sol:361

[NC-03] Use bytes.concat() and string.concat()

Solidity version 0.8.4 introduces:

• bytes.concat() vs abi.encodePacked(<bytes>,<bytes>)
• string.concat() vs abi.encodePacked(<string>,<string>)

https://docs.soliditylang.org/en/v0.8.17/types.html?highlight=bytes.concat#the-functions-bytes-concat-and-string-concat

Lines of code

• Vault.sol:35
• Vault.sol:46
• WithdrawProxy.sol:110
• WithdrawProxy.sol:124
• CollateralToken.sol:569
• PublicVault.sol:83
• PublicVault.sol:93
• PublicVault.sol:222
• AstariaRouter.sol:733
• ERC20-Cloned.sol:123
• VaultImplementation.sol:187

[NC-04] Lines are too long

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length.

Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length

Lines of code

• WithdrawProxy.sol:54
• WithdrawProxy.sol:56
• AstariaRouter.sol:75
• LienToken.sol:42
• VaultImplementation.sol:282
• VaultImplementation.sol:363

[NC-05] Empty blocks should be removed or Emit something

Lines of code

• ClearingHouse.sol:104
• ClearingHouse.sol:180

Recommended Mitigation Steps

The empty blocks should do something useful, such as emitting an event or reverting.

[NC-06] Reusable require statements should be changed to a modifier

Lines of code

• VaultImplementation.sol:78
• VaultImplementation.sol:96
• VaultImplementation.sol:105
• VaultImplementation.sol:114
• VaultImplementation.sol:147
• VaultImplementation.sol:211

Recommended Mitigation Steps

modifier onlyOwner() {
    require(msg.sender == owner());
    _;
}

[NC-07] Use a more recent version of OpenZeppelin dependencies

For security, it is best practice to use the latest OpenZeppelin version.

  "name": "openzeppelin-solidity",
  "description": "Secure Smart Contract library for Solidity",
  "version": "4.6.0",

Lines of code

• package.json:4

Recommended Mitigation Steps

Old version of OpenZeppelin is used (4.6.0), newer version can be used (4.7.3).

[NC-08] Critical changes should use-two step procedure

The CollateralToken.sol inherits the Solmate Auth.sol contract, which does not have a two-step procedure for critical changes.

    function transferOwnership(address newOwner) public virtual requiresAuth {
        owner = newOwner;

        emit OwnershipTransferred(msg.sender, newOwner);
    }

Lines of code

• CollateralToken.sol:27

Recommended Mitigation Steps

Consider adding two step procedure on the critical functions where the first is announcing a pending new owner and the new address should then claim its ownership.

[NC-09] Include return parameters in NatSpec comments

If Return parameters are declared, you must prefix them with /// @return. Some code analysis programs do analysis by reading NatSpec details, if they can't see the @return tag, they do incomplete analysis.

Recommended Mitigation Steps

Include the @return argument in the NatSpec comments.

[NC-10] Function writing does not comply with the Solidity Style Guide

Ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this.

Functions should be grouped according to their visibility and ordered:

• constructor()
• receive()
• fallback()
• external / public / internal / private
• view / pure

Recommended Mitigation Steps

Follow Solidity Style Guide.

[NC-11] The protocol should include NatSpec

It is recommended that Solidity contracts are fully annotated using NatSpec, it is clearly stated in the Solidity official documentation.

• In complex projects such as Defi, the interpretation of all functions and their arguments and returns is important for code readability and auditability.

• Some code analysis programs do analysis by reading NatSpec details, if they can't see the tags (@param, @dev, @return), they do incomplete analysis.

Lines of code

• ClearingHouse.sol:180
• ClearingHouse.sol:36
• WithdrawProxy.sol:230
• CollateralToken.sol:412
• PublicVault.sol:611
• PublicVault.sol:490
• AstariaRouter.sol:273
• LienToken.sol:265

Recommended Mitigation Steps

Include NatSpec comments in the codebase.

[NC-12] Avoid shadowing inherited state variables

In PublicVault.sol there is a local variable named owner, but there is a function named owner() in the inherited AstariaVaultBase.sol with the same name. This use causes compilers to issue warnings, negatively affecting checking and code readability.

  function owner() public pure returns (address) {
    return _getArgAddress(21); //ends at 44
  }

Lines of code

• AstariaVaultBase.sol:36
• PublicVault.sol:120 / PublicVault.sol:129 / PublicVault.sol:141 / PublicVault.sol:152

Recommended Mitigation Steps

Avoid using variables with the same name.