Astaria contest - synackrst's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L114 https://github.com/code-423n4/2023-01-astaria/blob/main/src/ClearingHouse.sol#L172

Vulnerability details

In ClearingHouse.sol, _execute settles auction for collateralId by paying currentOfferPrice amount of paymentToken in the contract’s balance. However, paymentToken can be arbitrarily specified via the public safeTransferFrom method, and there is no check to ensure that it is using the expected payment token.

Impact

Anyone can settle auction using an arbitrary ERC20 token, which will be used to pay debt and liquidator payment, and the corresponding collateral token will be burned. The future, legitimate settlement attempt will fail due to invalid collateral state.

Proof of Concept

• Deploy a new ERC20 token and set the clearing house’s balance to be above currentOfferPrice
• Call ClearingHouse’s safeTransferFrom(from, to, identifier, amount) where identifier is the ERC20 token’s address and the other arguments can be anything.

Tools Used

Manual analysis

Recommended Mitigation Steps

Include a check to ensure that it is using the expected paymentToken . In addition, implement access controls for safeTransferFrom.

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L27 https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626-Cloned.sol#L43

Vulnerability details

The following require statement In deposit of ERC4626-Cloned.sol

require(shares > minDepositAmount(), "VALUE_TOO_SMALL");

is ensuring shares is greater than minDepositAmount(), even though it should be ensuring assets > minDepositAmount() (like how it’s done in the mint method).

Impact

An unexpected amount of assets can be deposited; or a valid amount of assets cannot be deposited.

Proof of Concept

See the attached links.

Tools Used

Manual analysis

Recommended Mitigation Steps

Fix the require statement with the right invariant.

Low risk / QA findings

QA-1: Lack of documentation in initialize() of AstariaRouter

Function initalize() of AstariaRouter.sol lacks in documentation for the following arguments:

• _WITHDRAW_IMPL
• _CLEARING_HOUSE_IMPL
• _BEACON_PROXY_IMPL

QA-2: ETH accidentally sent to AstariaRouter cannot be recovered

AstariaRouter has multiple payable public methods: mint(), deposit(), withdraw(), redeem(), and pullToken(). ETH accidentally sent to this contract via these methods cannot be recovered unless the contract is selfdestructed or upgraded.

QA-3: Remove unused variables in liquidatorNFTClaim of CollateralToken.sol

The following variables are not used and hence can be removed.

address tokenContract = underlying.tokenContract;
uint256 tokenId = underlying.tokenId;

QA-4: Provide descriptive error info in PublicVault.sol

In multiple locations in PublicVault.sol, require statements lack in descriptive error information.

require(s.allowList[receiver]);  // in mint()

require(s.allowList[receiver]); // in deposit()

Add an error description in the second argument, or create a custom error type such as error NotInAllowList() and throw it with revert.

QA-5: Provide descriptive error info in VaultImplementation.sol

In multiple locations in VaultImplementation.sol, require statements lack in descriptive error messages.

require(msg.sender == owner()); //owner is "strategist"

Add an error description in the second argument, or create a custom error type and throw it with revert.

QA-6: Unused field in RouterStorage of IAstariaRouter.sol

The field ERC20 WETH in the RouterStorage struct is not being used anywhere in the code. This can be removed to reduce the storage space.

QA-7: Implicitly assumed non-inclusivity of minDepositAmount() in ERC4626-Cloned.sol

The following snippet

require(assets > minDepositAmount(), "VALUE_TOO_SMALL");

in the deposit and mint functions implicitly assumes that minDepositAmount is non-inclusive. Clarify this in the documentation.

QA-8: Remove unused variable in deposit of PublicVault.sol

uint256 assets = totalAssets();

is not used and hence can be removed.

Gas optimization

G-1: Use custom errors instead of revert strings

function getImpl(uint8 implType) external view returns (address impl) {
    impl = _loadRouterSlot().implementations[implType];
    if (impl == address(0)) {
      revert("unsupported/impl");
    }
  }

When impl contract is not defined, the reverting transaction would consume more gas because it’s not using custom errors;

G-2: Use assembly to fill the output array of balanceOfBatch in ClearingHouse.sol

Replace

function balanceOfBatch(address[] calldata accounts, uint256[] calldata ids)
    external
    view
    returns (uint256[] memory output)
  {
    output = new uint256[](accounts.length);
    for (uint256 i; i < accounts.length; ) {
      output[i] = type(uint256).max;
      unchecked {
        ++i;
      }
    }
  }

with

function balanceOfBatch(address[] calldata accounts, uint256[] calldata ids) public view returns (uint256[] memory output) {
    assembly {
        // Set the length of the output array
        let len := mload(accounts)
        mstore(output, len)

        // Set every bit of the output array to 1
        let i := 0
        for {
            lt(i, len)
        } {
            mstore8(add(output, add(i, 32)), 255)
            i := add(i, 32)
        }
    }
}

G-3: Use != 0 instead of > 0 in _execute of ClearingHouse.sol

The following snippet

if (ERC20(paymentToken).balanceOf(address(this)) > 0) {
      ERC20(paymentToken).safeTransfer(
        ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),
        ERC20(paymentToken).balanceOf(address(this))
      );
    }

can be replaced with

if (ERC20(paymentToken).balanceOf(address(this)) != 0) {
      ERC20(paymentToken).safeTransfer(
        ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),
        ERC20(paymentToken).balanceOf(address(this))
      );
    }

G-4: Unnecessary default value in _paymentAH of LienToken.sol

In the following snippet in _paymentAH() of LienToken.sol, the variable remaining is unnecessarily initialized with a default value.

uint256 remaining = 0;
if (owing > payment.safeCastTo88()) {
  remaining = owing - payment;
} else {
  payment = owing;
}

This can be rewritten in the following manner to reduce the gas cost.

uint256 remaining;
if (owing > payment.safeCastTo88()) {
  remaining = owing - payment;
} else {
  payment = owing;
}

G-5: Use temporary variable in _getPayee of LienToken.sol

function _getPayee(
    LienStorage storage s,
    uint256 lienId
  ) internal view returns (address) {
    return
      s.lienMeta[lienId].payee != address(0)
        ? s.lienMeta[lienId].payee
        : ownerOf(lienId);
  }

should be rewritten in the following manner to reduce the gas cost:

function _getPayee(LienStorage storage s, uint256 lienId) internal view returns (address) {
    address payee = s.lienMeta[lienId].payee;
    if (payee != address(0)) {
        return payee;
    } else {
        return ownerOf(lienId);
    }
}

G-6: Use != 0 instead of > 0 in processEpoch of PublicVault.sol

The following snippet

if (s.withdrawReserve > 0) {
      revert InvalidState(InvalidStates.WITHDRAW_RESERVE_NOT_ZERO);
}

can be rewritten in the following manner to reduce the gas cost.

if (s.withdrawReserve != 0) {
      revert InvalidState(InvalidStates.WITHDRAW_RESERVE_NOT_ZERO);
}

G-7: Assign liquidationWithdrawRatio in the else statement in processEpoch of PublicVault.sol

The following snippet

s.liquidationWithdrawRatio = 0;

// check if there are LPs withdrawing this epoch
if ((address(currentWithdrawProxy) != address(0))) {
  uint256 proxySupply = currentWithdrawProxy.totalSupply();

  s.liquidationWithdrawRatio = proxySupply
    .mulDivDown(1e18, totalSupply())
    .safeCastTo88();

  currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);
  uint256 expected = currentWithdrawProxy.getExpected();

  unchecked {
    if (totalAssets() > expected) {
      s.withdrawReserve = (totalAssets() - expected)
        .mulWadDown(s.liquidationWithdrawRatio)
        .safeCastTo88();
    } else {
      s.withdrawReserve = 0;
    }
  }
 
  _setYIntercept(
    s,
    s.yIntercept -
      totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)
  );
  // burn the tokens of the LPs withdrawing
  _burn(address(this), proxySupply);
}

should be rewritten in the following manner to save the gas cost (fewer sstore when currentWithdrawProxy is set)

// check if there are LPs withdrawing this epoch
if ((address(currentWithdrawProxy) != address(0))) {
  uint256 proxySupply = currentWithdrawProxy.totalSupply();

  s.liquidationWithdrawRatio = proxySupply
    .mulDivDown(1e18, totalSupply())
    .safeCastTo88();

  currentWithdrawProxy.setWithdrawRatio(s.liquidationWithdrawRatio);
  uint256 expected = currentWithdrawProxy.getExpected();

  unchecked {
    if (totalAssets() > expected) {
      s.withdrawReserve = (totalAssets() - expected)
        .mulWadDown(s.liquidationWithdrawRatio)
        .safeCastTo88();
    } else {
      s.withdrawReserve = 0;
    }
  }
 
  _setYIntercept(
    s,
    s.yIntercept -
      totalAssets().mulDivDown(s.liquidationWithdrawRatio, 1e18)
  );
  // burn the tokens of the LPs withdrawing
  _burn(address(this), proxySupply);
} else {
	s.liquidationWithdrawRatio = 0;
}

G-8: Use != 0 instead of > 0 in _beforeCommitToLien of PublicVault.sol

The following snippet

if (s.withdrawReserve > uint256(0)) {
      transferWithdrawReserve();
    }

can be rewritten in the following manner to reduce the gas cost

if (s.withdrawReserve != uint256(0)) {
      transferWithdrawReserve();
    }

G-9: Unnecessary mulDivDown in totalAssets() of PublicVault.sol

The following snippet

return uint256(s.slope).mulDivDown(delta_t, 1) + uint256(s.yIntercept);

can be replaced with

return uint256(s.slope) * delta_t + uint256(s.yIntercept);

G-10: Use more efficient increments/decrements in PublicVault.sol

i++ and i-- can be rewritten as ++i and --i respectively to reduce the gas cost in the following snippets in PublicVault.sol:

s.epochData[epoch].liensOpenForEpoch--; // in _decreaseEpochLienCount

s.epochData[epoch].liensOpenForEpoch++; // in _increaseOpenLiens

They can also be safely wrapped in unchecked to disable the overflow/underflow checks.

This can be applied to the following snippet as well:

unchecked {
      s.currentEpoch++;
}  // in processEpoch

G-11: Use more efficient increments/decrements in VaultImplementation.sol

i++ and i-- can be rewritten as ++i and --i respectively to reduce the gas cost in the following snippets in VaultImplementation.sol:

s.strategistNonce++; // in incrementNonce()

They can also be safely wrapped in unchecked to disable the overflow/underflow checks.

G-12: Use inline statement in settleAuction of CollateralToken.sol

_settleAuction(s, collateralId);

does not do much inside and can be replaced with the following to reduce a function call.