Astaria contest - Aymen0909's results

QA Report

Summary

Findings

1- Non allowed users are able to mint and deposit in PublicVault :

Both mint and deposit functions allow any user to mint new shares or deposit assets if they know and address in the allow list (which can be obtained from on-chain data) which should not be allowed as only the addresses in the allowList should be able to call the mint and deposit functions.

Even though the deposited assets or the minted shares goes to the receiver (who is in the allowed list), the impact of this is that a malicious user can keep calling the deposit function for example by sending some assets, the deposit function will call the afterDeposit hook which updates the s.yIntercept value, and so the yIntercept will keep changing in value which can impact the protocol, not to forget that there is a deposit cap that can be reached.

I submitted this as Low because the malicious user must lose money to perform this "attack", but it's still valid because in the protocol logic it's said that only addresses from the allowList should be able to call the mint and deposit functions.

Risk : Low

Proof of Concept

Instances occurs in the code below :

File: src/PublicVault.sol Line 233-244

function mint(uint256 shares, address receiver)
    public
    override(ERC4626Cloned)
    whenNotPaused
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
    if (s.allowListEnabled) {
      require(s.allowList[receiver]);
    }
    return super.mint(shares, receiver);
  }

File: src/PublicVault.sol Line 251-265

  function deposit(uint256 amount, address receiver)
    public
    override(ERC4626Cloned)
    whenNotPaused
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
    if (s.allowListEnabled) {
      require(s.allowList[receiver]);
    }

    uint256 assets = totalAssets();

    return super.deposit(amount, receiver);
  }

As you can see both functions can be called by any user because the only check is require(s.allowList[receiver]) which can be bypassed by getting an allowed address from the on-chain storage, and in the contract logic it is intended that only addresses from the allowList should be able to call the mint and deposit functions.

Mitigation

To avoid this isssue, the check on the receiver address should be replaced in both mint and deposit functions by msg.sender :

function mint(uint256 shares, address receiver)
    public
    override(ERC4626Cloned)
    whenNotPaused
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
    if (s.allowListEnabled) {
      require(s.allowList[msg.sender]);
    }
    return super.mint(shares, receiver);
  }

  function deposit(uint256 amount, address receiver)
    public
    override(ERC4626Cloned)
    whenNotPaused
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
    if (s.allowListEnabled) {
      require(s.allowList[msg.sender]);
    }

    uint256 assets = totalAssets();

    return super.deposit(amount, receiver);
  }

2- Wrong named return variable declared :

Risk : NON CRITICAL

The mint function from the WithdrawProxy contract is supposed to return the minted shares but a wrong return variable asset is declared instead of shares.

File: src/WithdrawProxy.sol Line 132-141

function mint(uint256 shares, address receiver)
    public
    virtual
    override(ERC4626Cloned, IERC4626)
    onlyVault
    returns (uint256 assets)
{
    require(msg.sender == VAULT(), "only vault can mint");
    _mint(receiver, shares);
    return shares;
}

Mitigation

The mint function should be modified as follow :

function mint(uint256 shares, address receiver)
    public
    virtual
    override(ERC4626Cloned, IERC4626)
    returns (uint256 shares)
{
    _mint(receiver, shares);
}

3- Use existing modifier instead of add additional require checks :

Risk : NON CRITICAL

The mint function from the WithdrawProxy contract should use the existing onlyVault modifier instead of declaring a duplicate require check statement.

File: src/WithdrawProxy.sol Line 132-141

function mint(uint256 shares, address receiver)
    public
    virtual
    override(ERC4626Cloned, IERC4626)
    onlyVault
    returns (uint256 assets)
{
    require(msg.sender == VAULT(), "only vault can mint");
    _mint(receiver, shares);
    return shares;
}

Mitigation

The mint function should be modified as follow :

function mint(uint256 shares, address receiver)
    public
    virtual
    override(ERC4626Cloned, IERC4626)
    returns (uint256 assets)
{
    _mint(receiver, shares);
    return shares;
}

4- Duplicated require() checks should be refactored to a modifier :

require() checks statement used multiple times inside a contract should be refactored to a modifier for better readability.

Risk : NON CRITICAL

Proof of Concept

There are 6 instances of this issue in VaultImplementation.sol:

File: src/VaultImplementation.sol

78      require(msg.sender == owner())
96      require(msg.sender == owner())
105     require(msg.sender == owner())
114     require(msg.sender == owner())
147     require(msg.sender == owner())
211     require(msg.sender == owner())

Those checks should be replaced by a onlyOwner modifier as follow :

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

There are 3 instances of this issue in AstariaRouter.sol:

File: src/AstariaRouter.sol

341     require(msg.sender == s.guardian);
347     require(msg.sender == s.guardian);
361     require(msg.sender == address(s.guardian));

Those checks should be replaced by a OnlyGuardien modifier as follow :

modifier OnlyGuardien(){
    require(msg.sender == s.guardian);
    _;
}

There are 3 instances of this issue in ClearingHouse.sol:

File: src/ClearingHouse.sol

199     require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
216     require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
223     require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));

Those checks should be replaced by a OnlyCollateral modifier as follow :

modifier OnlyCollateral(){
    IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0));
    require(msg.sender == address(ASTARIA_ROUTER.COLLATERAL_TOKEN()));
    _;
}

5- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: src/AstariaRouter.sol Line 721

returns (address vaultAddr)

File: src/CollateralToken.sol Line 162

returns (bytes4 validOrderMagicValue)

File: src/CollateralToken.sol Line 177

returns (bytes4 validOrderMagicValue)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

Gas Optimizations

Summary

Findings

1- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 5 instances of this issue :

File: src/WithdrawProxy.sol Line 247-286

In the code linked above the function VAULT() is called multiple times, this function reads from storage the address of the Vault and because this address won't be changed in those lines of code the function VAULT() should be called at the beginning and the Vault address should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: src/WithdrawProxy.sol Line 258-271

In the code linked above the values of s.withdrawRatio and s.expected are read multiple times (more than 3) from storage and their respective values does not change so they should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: src/CollateralToken.sol Line 299-325

In the code linked above the value of s.clearingHouse[collateralId] is read multiple times (3) from storage and it's respective values does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: src/CollateralToken.sol Line 451-463

In the code linked above the value of s.clearingHouse[collateralId] is read multiple times (4) from storage and it's respective values does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

File: src/CollateralToken.sol Line 531-535

In the code linked above the value of s.clearingHouse[collateralId] is read multiple times (2) from storage and it's respective values does not change so it should be cached into a memory variable in order to save gas by avoiding multiple reads from storage.

2- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 2 instances of this issue:

File: src/LienToken.sol Line 638

remaining = owing - payment;

The above operation cannot underflow due to the check :

File: src/LienToken.sol Line 637

if (owing > payment.safeCastTo88())

So the operation should be marked as unchecked.

File: src/LienToken.sol Line 830

stack.point.amount -= amount.safeCastTo88();

The above operation cannot underflow due to the check :

File: src/LienToken.sol Line 829

if (stack.point.amount > amount)

So the operation should be marked as unchecked.

3- Remove redundant checks :

In the function below there are two collateral owner checks (modifier + if check), this is redundant and should be removed to save gas.

File: src/CollateralToken.sol Line 331-346

function releaseToAddress(uint256 collateralId, address releaseTo)
public
releaseCheck(collateralId)
onlyOwner(collateralId)
{
    CollateralStorage storage s = _loadCollateralSlot();

    if (msg.sender != ownerOf(collateralId)) {
        revert InvalidSender();
    }
    Asset memory underlying = s.idToUnderlying[collateralId];
    address tokenContract = underlying.tokenContract;
    _burn(collateralId);
    delete s.idToUnderlying[collateralId];
    _releaseToAddress(s, underlying, collateralId, releaseTo);
}

4- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 8 instances of this issue:

File: src/WithdrawProxy.sol

237     s.withdrawReserveReceived += amount;
313     s.expected += newLienExpectedValue.safeCastTo88();

File: src/PublicVault.sol

380     s.withdrawReserve -= withdrawBalance.safeCastTo88();
405     s.withdrawReserve -= drainBalance.safeCastTo88();
565     s.slope += computedSlope.safeCastTo48();
583     s.yIntercept += assets.safeCastTo88();
606     s.strategistUnclaimedShares += feeInShares;
624     s.yIntercept += params.increaseYIntercept.safeCastTo88();

5- Splitting require() statements that uses && saves gas :

There are 3 instances of this issue :

File: src/Vault.sol

65      require(s.allowList[msg.sender] && receiver == owner());

File: src/LienToken.sol

672     require(
            currentEpoch != 0 &&
            msg.sender == s.epochData[currentEpoch - 1].withdrawProxy
        );
687     require(
            currentEpoch != 0 &&
            msg.sender == s.epochData[currentEpoch - 1].withdrawProxy
        );