Astaria contest - ladboy233's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L169 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L123

Vulnerability details

Impact

Lack of access control for ClearingHouse.sol#safeTransferFrom and lack of validation for payment token when settling the auction

Proof of Concept

The function ClearingHouse#safeTransferFrom is meant to settle the auction but the function severely lack of access control

  function safeTransferFrom(
    address from, // the from is the offerer
    address to,
    uint256 identifier,
    uint256 amount,
    bytes calldata data //empty from seaport
  ) public {
    //data is empty and useless
    _execute(from, to, identifier, amount);
  }

which calls:

  function _execute(
    address tokenContract, // collateral token sending the fake nft
    address to, // buyer
    uint256 encodedMetaData, //retrieve token address from the encoded data
    uint256 // space to encode whatever is needed,
  ) internal {
    IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0)); // get the router from the immutable arg

    ClearingHouseStorage storage s = _getStorage();
    address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

    uint256 currentOfferPrice = _locateCurrentAmount({
      startAmount: s.auctionStack.startAmount,
      endAmount: s.auctionStack.endAmount,
      startTime: s.auctionStack.startTime,
      endTime: s.auctionStack.endTime,
      roundUp: true //we are a consideration we round up
    });
    uint256 payment = ERC20(paymentToken).balanceOf(address(this));

    require(payment >= currentOfferPrice, "not enough funds received");

    uint256 collateralId = _getArgUint256(21);
    // pay liquidator fees here

    ILienToken.AuctionStack[] storage stack = s.auctionStack.stack;

    uint256 liquidatorPayment = ASTARIA_ROUTER.getLiquidatorFee(payment);

    ERC20(paymentToken).safeTransfer(
      s.auctionStack.liquidator,
      liquidatorPayment
    );

    ERC20(paymentToken).safeApprove(
      address(ASTARIA_ROUTER.TRANSFER_PROXY()),
      payment - liquidatorPayment
    );

    ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
      paymentToken,
      collateralId,
      payment - liquidatorPayment,
      s.auctionStack.stack
    );

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

An adversay can exploit the lack of input validation for encodedMetaData, which can derive the payment token address

ClearingHouseStorage storage s = _getStorage();
address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

uint256 currentOfferPrice = _locateCurrentAmount({
  startAmount: s.auctionStack.startAmount,
  endAmount: s.auctionStack.endAmount,
  startTime: s.auctionStack.startTime,
  endTime: s.auctionStack.endTime,
  roundUp: true //we are a consideration we round up
});
uint256 payment = ERC20(paymentToken).balanceOf(address(this));

require(payment >= currentOfferPrice, "not enough funds received");

the payment token address here should match the settlement token.

We can look into the liquidation flow:

first, the liquidate function is called in AstariaRouter.sol

  function liquidate(ILienToken.Stack[] memory stack, uint8 position)
    public
    returns (OrderParameters memory listedOrder)
  {
    if (!canLiquidate(stack[position])) {
      revert InvalidLienState(LienState.HEALTHY);
    }

    RouterStorage storage s = _loadRouterSlot();
    uint256 auctionWindowMax = s.auctionWindow + s.auctionWindowBuffer;

    s.LIEN_TOKEN.stopLiens(
      stack[position].lien.collateralId,
      auctionWindowMax,
      stack,
      msg.sender
    );
    emit Liquidation(stack[position].lien.collateralId, position);
    listedOrder = s.COLLATERAL_TOKEN.auctionVault(
      ICollateralToken.AuctionVaultParams({
        settlementToken: stack[position].lien.token,
        collateralId: stack[position].lien.collateralId,
        maxDuration: auctionWindowMax,
        startingPrice: stack[0].lien.details.liquidationInitialAsk,
        endingPrice: 1_000 wei
      })
    );
  }

then function auctionVault is called in CollateralToken

function auctionVault(AuctionVaultParams calldata params)
	external
	requiresAuth
returns (OrderParameters memory orderParameters)
{
CollateralStorage storage s = _loadCollateralSlot();

uint256[] memory prices = new uint256[](2);
prices[0] = params.startingPrice;
prices[1] = params.endingPrice;
orderParameters = _generateValidOrderParameters(
  s,
  params.settlementToken,
  params.collateralId,
  prices,
  params.maxDuration
);

_listUnderlyingOnSeaport(
  s,
  params.collateralId,
  Order(orderParameters, new bytes(0))
);
}

the function _generateValidOrderParameters is important:

  function _generateValidOrderParameters(
    CollateralStorage storage s,
    address settlementToken,
    uint256 collateralId,
    uint256[] memory prices,
    uint256 maxDuration
  ) internal returns (OrderParameters memory orderParameters) {
    OfferItem[] memory offer = new OfferItem[](1);

    Asset memory underlying = s.idToUnderlying[collateralId];

    offer[0] = OfferItem(
      ItemType.ERC721,
      underlying.tokenContract,
      underlying.tokenId,
      1,
      1
    );

    ConsiderationItem[] memory considerationItems = new ConsiderationItem[](2);
    considerationItems[0] = ConsiderationItem(
      ItemType.ERC20,
      settlementToken,
      uint256(0),
      prices[0],
      prices[1],
      payable(address(s.clearingHouse[collateralId]))
    );
    considerationItems[1] = ConsiderationItem(
      ItemType.ERC1155,
      s.clearingHouse[collateralId],
      uint256(uint160(settlementToken)),
      prices[0],
      prices[1],
      payable(s.clearingHouse[collateralId])
    );

    orderParameters = OrderParameters({
      offerer: s.clearingHouse[collateralId],
      zone: address(this), // 0x20
      offer: offer,
      consideration: considerationItems,
      orderType: OrderType.FULL_OPEN,
      startTime: uint256(block.timestamp),
      endTime: uint256(block.timestamp + maxDuration),
      zoneHash: bytes32(collateralId),
      salt: uint256(blockhash(block.number)),
      conduitKey: s.CONDUIT_KEY, // 0x120
      totalOriginalConsiderationItems: considerationItems.length
    });
  }

note the first consideration item:

ConsiderationItem[] memory considerationItems = new ConsiderationItem[](2);
considerationItems[0] = ConsiderationItem(
  ItemType.ERC20,
  settlementToken,
  uint256(0),
  prices[0],
  prices[1],
  payable(address(s.clearingHouse[collateralId]))
);

the settlementToken is set when the order is created, which matching the underlying token in ERC4626 vault, but when the auction is settled, there is no such validation to make sure the derived payment token match the settlementToken.

ClearingHouseStorage storage s = _getStorage();
address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

uint256 currentOfferPrice = _locateCurrentAmount({
  startAmount: s.auctionStack.startAmount,
  endAmount: s.auctionStack.endAmount,
  startTime: s.auctionStack.startTime,
  endTime: s.auctionStack.endTime,
  roundUp: true //we are a consideration we round up
});
uint256 payment = ERC20(paymentToken).balanceOf(address(this));

require(payment >= currentOfferPrice, "not enough funds received");

Now we can formalize a exploit path:

1. An adversary create a worthless ERC20 token and airdrop / mint worthless token to the ClearingHouse contract.
2. A adversary call safeTransferFrom in ClearningHouse

  function safeTransferFrom(
    address from, // the from is the offerer
    address to,
    uint256 identifier,
    uint256 amount,
    bytes calldata data //empty from seaport
  ) public {
    //data is empty and useless
    _execute(from, to, identifier, amount);
  }

the third parameter identifier is decoded as the worthless token.

3. the code below executes because the payment token is worthless and the adversary already airdrop the token into the contract to make sure the balanceOf check can pass

require(payment >= currentOfferPrice, "not enough funds received");

ClearingHouseStorage storage s = _getStorage();
address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

uint256 currentOfferPrice = _locateCurrentAmount({
  startAmount: s.auctionStack.startAmount,
  endAmount: s.auctionStack.endAmount,
  startTime: s.auctionStack.startTime,
  endTime: s.auctionStack.endTime,
  roundUp: true //we are a consideration we round up
});
uint256 payment = ERC20(paymentToken).balanceOf(address(this));

require(payment >= currentOfferPrice, "not enough funds received");

4. the auction is settled in worthless token, the oustanding debt is never paid off and the lender has to bear the loss

    uint256 collateralId = _getArgUint256(21);
    // pay liquidator fees here

    ILienToken.AuctionStack[] storage stack = s.auctionStack.stack;

    uint256 liquidatorPayment = ASTARIA_ROUTER.getLiquidatorFee(payment);

    ERC20(paymentToken).safeTransfer(
      s.auctionStack.liquidator,
      liquidatorPayment
    );

    ERC20(paymentToken).safeApprove(
      address(ASTARIA_ROUTER.TRANSFER_PROXY()),
      payment - liquidatorPayment
    );

    ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
      paymentToken,
      collateralId,
      payment - liquidatorPayment,
      s.auctionStack.stack
    );

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

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol validate the caller of the safeTransferFrom in ClearingHouse is the seaport / conduict contract and validate the payment token that settle the auction match the settlement ERC20 token in the CollateralToken and ERC46262 vault underlying asset.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/Vault.sol#L72

Vulnerability details

Impact

Incorrect usage of safeTransferFrom revert Vault#withdraw

Proof of Concept

Note the function withdraw in the vault.

function withdraw(uint256 amount) external {
	require(msg.sender == owner());
	ERC20(asset()).safeTransferFrom(address(this), msg.sender, amount);
}

This incorrect usage of safeTransferFrom rever the withdraw transaction.

Because the Vault never gives approval for ERC20 transfers, calls to safeTransferFrom on the asset() will revert with insufficient approval

The root cause of this issue is misusing safeTransferFrom to transfer tokens directly out of the contract instead of using transfer directly. The contract will hold the token balance and thus does not need approval to transfer tokens, nor can it approve token transfers in the current implementation.

The coded POC is below:

First, please add this test in AstariaTest.t.sol

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/test/AstariaTest.t.sol#L72

  function testTransferFromWETH_Revert_POC() public {
    address privateVault = _createPrivateVault({
      strategist: strategistOne,
      delegate: strategistTwo
    });

    address owner = VaultImplementation(privateVault).owner();
    
    WETH9.deposit{value: 1 ether}();
    WETH9.transfer(privateVault, 1 ether);
    console.log(WETH9.balanceOf(privateVault));
  
    vm.prank(owner);
    Vault(privateVault).withdraw(1);
  }

the code just try to send some WETH to the vault contract and try to withdraw the asset,

we run the test

forge test -vv --match testTransferFromWETH_Revert_POC

and the test revert.

Encountered 1 failing test in src/test/AstariaTest.t.sol:AstariaTest
[FAIL. Reason: TRANSFER_FROM_FAILED] testTransferFromWETH_Revert_POC() (gas: 923788)

We change from

function withdraw(uint256 amount) external {
	require(msg.sender == owner());
	ERC20(asset()).safeTransferFrom(address(this), msg.sender, amount);
}

to

function withdraw(uint256 amount) external {
	require(msg.sender == owner());
	ERC20(asset()).safeTransfer(msg.sender, amount);
}

note we use safeTransfer instead of safeTransferFrom and we run the test, the test pass.

Running 1 test for src/test/AstariaTest.t.sol:AstariaTest
[PASS] testTransferFromWETH_Revert_POC() (gas: 285468)
Logs:
  1000000000000000000

Test result: ok. 1 passed; 0 failed; finished in 14.80ms

Tools Used

Manual Review, Foundary

Recommended Mitigation Steps

We recommend the protocol use safeTransfer instead of safeTransferFrom in the withdraw function in the Vault contract.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L246 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L184

Vulnerability details

Impact

VaultImplementation#_validateCommitment signature commit proof message can be reused / replayed because the lack of nonce increment

Proof of Concept

VaultImplementation#_validateCommitment signature commit proof message can be reused / replayed because the lack of nonce check

VIData storage s = _loadVISlot();
address recovered = ecrecover(
  keccak256(
	_encodeStrategyData(
	  s,
	  params.lienRequest.strategy,
	  params.lienRequest.merkle.root
	)
  ),
  params.lienRequest.v,
  params.lienRequest.r,
  params.lienRequest.s
);
if (
  (recovered != owner() && recovered != s.delegate) ||
  recovered == address(0)
) {
  revert IVaultImplementation.InvalidRequest(
	InvalidRequestReason.INVALID_SIGNATURE
  );
}

this implementation is vulnerable to signature reuse / replay attack.

We need to check encodeStrategyData

function _encodeStrategyData(
VIData storage s,
IAstariaRouter.StrategyDetailsParam calldata strategy,
bytes32 root
) internal view returns (bytes memory) {
bytes32 hash = keccak256(
  abi.encode(STRATEGY_TYPEHASH, s.strategistNonce, strategy.deadline, root)
);
return
  abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), hash);
}

We use IAstariaRouter(ROUTER()).strategistNonce(strategy.strategist), but we does not increment the nonce each time after the commitment message is used.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the projected increment the nonce each time after the commitment is used in

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L644 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L645

Vulnerability details

Impact

.The best-effort one-time dutch auction for borrower collateral is not economically efficient to drive auction bids towards reaching the total outstanding debt, which leads to loss of LP funds.

Proof of Concept

When any lien against a borrower collateral is not paid within the lien duration, the underlying collateral is put up for dutch auction.

In AstairaRouter.sol

  function liquidate(ILienToken.Stack[] memory stack, uint8 position)
    public
    returns (OrderParameters memory listedOrder)
  {
    if (!canLiquidate(stack[position])) {
      revert InvalidLienState(LienState.HEALTHY);
    }

    RouterStorage storage s = _loadRouterSlot();
    uint256 auctionWindowMax = s.auctionWindow + s.auctionWindowBuffer;

    s.LIEN_TOKEN.stopLiens(
      stack[position].lien.collateralId,
      auctionWindowMax,
      stack,
      msg.sender
    );
    emit Liquidation(stack[position].lien.collateralId, position);
    listedOrder = s.COLLATERAL_TOKEN.auctionVault(
      ICollateralToken.AuctionVaultParams({
        settlementToken: stack[position].lien.token,
        collateralId: stack[position].lien.collateralId,
        maxDuration: auctionWindowMax,
        startingPrice: stack[0].lien.details.liquidationInitialAsk,
        endingPrice: 1_000 wei
      })
    );
  }

According to the doc:

https://docs.astaria.xyz/docs/protocol-mechanics/liquidations

Dutch auctions

A borrower has failed to repay their Lien Position. A liquidator will start the liquidation process.

The Dutch auction starts at the liquidationInitialAsk set by the strategist.

Auction lasts for a total of 72 hours or when an order is matched

The dutch auction is not guaranteed to cover the debt for a few reason:

first of all, the startingPrice: stack[0].lien.details.liquidationInitialAsk does not consider the opensea trading fee.

According to

https://support.opensea.io/hc/en-us/articles/1500011590241-What-are-service-fees-and-creator-earnings-

OpenSea's model is simple—we receive 2.5% of the sale price. That's it. Users and partners can create NFTs for free at any time.

Without considering the opensea fee, the initial auction price can be lower than the oustanding debt already, then liquidated NFT cannot cover the outstanding debt.

Secondly, the ending price 1000 WEI is too low.

startingPrice: stack[0].lien.details.liquidationInitialAsk,
endingPrice: 1_000 wei

Because the NFT is in dutch auction, the longer no one buy the NFT, the lower the price,

The startingPrice is liquidationInitialAsk, but if no one bought for the auction, the endingPrice is 1000 WEI, which is very unlikely a not sufficient auction price to cover the oustanding debt.

the ending price is like a slippage control, 1000 WEI is too low for lender to cover the debt.

Thirdly, if no one buy the NFT, auction ended and the NFT is claimed by liquidator instead of re-auction.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend set the price higher than oustanding debt at the initial price for auction in order to at least consider opensea trading fee and increase the endingPrice up to at least a high portion of oustanding debt instead of lending the strategist set the liquidationInitialAsk

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L274 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/CollateralToken.sol#L266 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L238

Vulnerability details

Impact

CollateralToken should allow to execute token owner's action to approved addresses

Proof of Concept

Let us look into the code below in VaultImplementation#_validateCommitment

function _validateCommitment(
IAstariaRouter.Commitment calldata params,
address receiver
) internal view {
uint256 collateralId = params.tokenContract.computeId(params.tokenId);
ERC721 CT = ERC721(address(COLLATERAL_TOKEN()));
address holder = CT.ownerOf(collateralId);
address operator = CT.getApproved(collateralId);
if (
  msg.sender != holder &&
  receiver != holder &&
  receiver != operator &&
  !CT.isApprovedForAll(holder, msg.sender)
) {
  revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
}

the code check should also check that msg.sender != operator to make the check complete, if the msg.sender comes from an approved operator, the call should be valid

if (
  msg.sender != holder &&
  receiver != holder &&
  receiver != operator &&
  msg.sender != operator &&
  !CT.isApprovedForAll(holder, msg.sender)
) {
  revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
}

AND

CollateralToken functions flashAction, releaseToAddress are restricted to the owner of token only. But they should be allowed for approved addresses as well.

For example, in flashAuction, only the owner of the collateral token can start the flashAction, then approved operator by owner cannot start flashAction

  function flashAction(
    IFlashAction receiver,
    uint256 collateralId,
    bytes calldata data
  ) external onlyOwner(collateralId) {

note the check onlyOwner(collateralId) does not check if the msg.sender is an approved operator.

modifier onlyOwner(uint256 collateralId) {
	require(ownerOf(collateralId) == msg.sender);
	_;
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add ability for approved operators to call functions that can be called by the collateral token owner.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L66 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L73

Vulnerability details

Impact

Lack of support for ERC20 token that is not 18 decimals in PublicVault.sol

Proof of Concept

We need to look into the PublicVault.sol implementation

contract PublicVault is VaultImplementation, IPublicVault, ERC4626Cloned {

the issue that is the decimal precision in the PublicVault is hardcoded to 18

function decimals()
	public
	pure
	virtual
	override(IERC20Metadata)
returns (uint8)
{
	return 18;
}

According to

https://eips.ethereum.org/EIPS/eip-4626

Although the convertTo functions should eliminate the need for any use of an EIP-4626 Vault’s decimals variable, it is still strongly recommended to mirror the underlying token’s decimals if at all possible, to eliminate possible sources of confusion and simplify integration across front-ends and for other off-chain users.

The solmate ERC4626 implementation did mirror the underlying token decimals

https://github.com/transmissions11/solmate/blob/3998897acb502fa7b480f505138a6ae1842e8d10/src/mixins/ERC4626.sol#L38

constructor(
	ERC20 _asset,
	string memory _name,
	string memory _symbol
) ERC20(_name, _symbol, _asset.decimals()) {
	asset = _asset;
}

but the token decimals is over-written to 18 decimals.

https://github.com/d-xo/weird-erc20#low-decimals

Some tokens have low decimals (e.g. USDC has 6). Even more extreme, some tokens like Gemini USD only have 2 decimals.

For example, if the underlying token is USDC and has 6 decimals, the convertToAssets() function will be broken.

https://github.com/transmissions11/solmate/blob/3998897acb502fa7b480f505138a6ae1842e8d10/src/mixins/ERC4626.sol#L130

function convertToAssets(uint256 shares) public view virtual returns (uint256) {
	uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

	return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
}

the totalSupply is in 18 deimals, but the totalAssets is in 6 deciimals, but the totalSupply should be 6 decimals as well to match the underlying token precision.

There are place tha the code assume the token is 18 decimals, if the token is not 18 decimals, the logic for liquidatoin ratio calculation is broken as well because the hardcoded 1e18 is used.

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)
);

and In the claim function for WithdrawProxy.sol

if (balance < s.expected) {
  PublicVault(VAULT()).decreaseYIntercept(
	(s.expected - balance).mulWadDown(1e18 - s.withdrawRatio)
  );
} else {
  PublicVault(VAULT()).increaseYIntercept(
	(balance - s.expected).mulWadDown(1e18 - s.withdrawRatio)
  );
}

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol make the PublicVault.sol decimal match the underlying token decimals.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L169 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/ClearingHouse.sol#L123

Vulnerability details

Impact

Adversary can game the liquidation flow by transfering a dust amount of the payment token to ClearingHouse contract to settle the auction

Proof of Concept

The function ClearingHouse#safeTransferFrom is meant to settle the auction but the function severely lack of access control

  function safeTransferFrom(
    address from, // the from is the offerer
    address to,
    uint256 identifier,
    uint256 amount,
    bytes calldata data //empty from seaport
  ) public {
    //data is empty and useless
    _execute(from, to, identifier, amount);
  }

which calls:

  function _execute(
    address tokenContract, // collateral token sending the fake nft
    address to, // buyer
    uint256 encodedMetaData, //retrieve token address from the encoded data
    uint256 // space to encode whatever is needed,
  ) internal {
    IAstariaRouter ASTARIA_ROUTER = IAstariaRouter(_getArgAddress(0)); // get the router from the immutable arg

    ClearingHouseStorage storage s = _getStorage();
    address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

    uint256 currentOfferPrice = _locateCurrentAmount({
      startAmount: s.auctionStack.startAmount,
      endAmount: s.auctionStack.endAmount,
      startTime: s.auctionStack.startTime,
      endTime: s.auctionStack.endTime,
      roundUp: true //we are a consideration we round up
    });
    uint256 payment = ERC20(paymentToken).balanceOf(address(this));

    require(payment >= currentOfferPrice, "not enough funds received");

    uint256 collateralId = _getArgUint256(21);
    // pay liquidator fees here

    ILienToken.AuctionStack[] storage stack = s.auctionStack.stack;

    uint256 liquidatorPayment = ASTARIA_ROUTER.getLiquidatorFee(payment);

    ERC20(paymentToken).safeTransfer(
      s.auctionStack.liquidator,
      liquidatorPayment
    );

    ERC20(paymentToken).safeApprove(
      address(ASTARIA_ROUTER.TRANSFER_PROXY()),
      payment - liquidatorPayment
    );

    ASTARIA_ROUTER.LIEN_TOKEN().payDebtViaClearingHouse(
      paymentToken,
      collateralId,
      payment - liquidatorPayment,
      s.auctionStack.stack
    );

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

We can look into the liquidation flow:

first, the liquidate function is called in AstariaRouter.sol

  function liquidate(ILienToken.Stack[] memory stack, uint8 position)
    public
    returns (OrderParameters memory listedOrder)
  {
    if (!canLiquidate(stack[position])) {
      revert InvalidLienState(LienState.HEALTHY);
    }

    RouterStorage storage s = _loadRouterSlot();
    uint256 auctionWindowMax = s.auctionWindow + s.auctionWindowBuffer;

    s.LIEN_TOKEN.stopLiens(
      stack[position].lien.collateralId,
      auctionWindowMax,
      stack,
      msg.sender
    );
    emit Liquidation(stack[position].lien.collateralId, position);
    listedOrder = s.COLLATERAL_TOKEN.auctionVault(
      ICollateralToken.AuctionVaultParams({
        settlementToken: stack[position].lien.token,
        collateralId: stack[position].lien.collateralId,
        maxDuration: auctionWindowMax,
        startingPrice: stack[0].lien.details.liquidationInitialAsk,
        endingPrice: 1_000 wei
      })
    );
  }

then function auctionVault is called in CollateralToken

function auctionVault(AuctionVaultParams calldata params)
	external
	requiresAuth
returns (OrderParameters memory orderParameters)
{
CollateralStorage storage s = _loadCollateralSlot();

uint256[] memory prices = new uint256[](2);
prices[0] = params.startingPrice;
prices[1] = params.endingPrice;
orderParameters = _generateValidOrderParameters(
  s,
  params.settlementToken,
  params.collateralId,
  prices,
  params.maxDuration
);

_listUnderlyingOnSeaport(
  s,
  params.collateralId,
  Order(orderParameters, new bytes(0))
);
}

the function _generateValidOrderParameters is important:

  function _generateValidOrderParameters(
    CollateralStorage storage s,
    address settlementToken,
    uint256 collateralId,
    uint256[] memory prices,
    uint256 maxDuration
  ) internal returns (OrderParameters memory orderParameters) {
    OfferItem[] memory offer = new OfferItem[](1);

    Asset memory underlying = s.idToUnderlying[collateralId];

    offer[0] = OfferItem(
      ItemType.ERC721,
      underlying.tokenContract,
      underlying.tokenId,
      1,
      1
    );

    ConsiderationItem[] memory considerationItems = new ConsiderationItem[](2);
    considerationItems[0] = ConsiderationItem(
      ItemType.ERC20,
      settlementToken,
      uint256(0),
      prices[0],
      prices[1],
      payable(address(s.clearingHouse[collateralId]))
    );
    considerationItems[1] = ConsiderationItem(
      ItemType.ERC1155,
      s.clearingHouse[collateralId],
      uint256(uint160(settlementToken)),
      prices[0],
      prices[1],
      payable(s.clearingHouse[collateralId])
    );

    orderParameters = OrderParameters({
      offerer: s.clearingHouse[collateralId],
      zone: address(this), // 0x20
      offer: offer,
      consideration: considerationItems,
      orderType: OrderType.FULL_OPEN,
      startTime: uint256(block.timestamp),
      endTime: uint256(block.timestamp + maxDuration),
      zoneHash: bytes32(collateralId),
      salt: uint256(blockhash(block.number)),
      conduitKey: s.CONDUIT_KEY, // 0x120
      totalOriginalConsiderationItems: considerationItems.length
    });
  }

note the first consideration item:

ConsiderationItem[] memory considerationItems = new ConsiderationItem[](2);
considerationItems[0] = ConsiderationItem(
  ItemType.ERC20,
  settlementToken,
  uint256(0),
  prices[0],
  prices[1],
  payable(address(s.clearingHouse[collateralId]))
);

prices[0] is the initialLiquidationPrice the starting price, prices[1] is the ending price, which is 1000 WEI. this means if no one buy the dutch auction, the price will fall to 1000 WEI.

ClearingHouseStorage storage s = _getStorage();
address paymentToken = bytes32(encodedMetaData).fromLast20Bytes();

uint256 currentOfferPrice = _locateCurrentAmount({
  startAmount: s.auctionStack.startAmount,
  endAmount: s.auctionStack.endAmount,
  startTime: s.auctionStack.startTime,
  endTime: s.auctionStack.endTime,
  roundUp: true //we are a consideration we round up
});
uint256 payment = ERC20(paymentToken).balanceOf(address(this));

require(payment >= currentOfferPrice, "not enough funds received");

In the code above, note that the code check if the current balance of the payment token is larger than currentOfferPrice computed by _locateCurrentAmount, this utility function comes from seaport.

https://github.com/ProjectOpenSea/seaport/blob/f402dac8b3faabdb8420d31d46759f47c9d74b7d/contracts/lib/AmountDeriver.sol#L38

If there is no one buy the dutch auction, the price will drop to until 1000 WEI, which means _locateCurrentAmount returns lower and lower price.

In normal flow, if no one buy the dutch auction and cover the outstanding debt, the NFT can be claimabled by liquidator. The liquidator can try to sell NFT again to cover the debt and loss for lenders,

However, if no one wants to buy the dutch auction and the _locateCurrentAmount is low enough, for example, 10000 WEI, an adversary can transfer 10001 WEI of ERC20 payment token to the ClearingHouse contract

Then call safeTransferFrom to settle the auction.

the code below will execute

uint256 payment = ERC20(paymentToken).balanceOf(address(this));

require(payment >= currentOfferPrice, "not enough funds received");

. beceause the airdropped ERC20 balance make the payment larger than currentOfferPrice

In this case, the adversary blocks the liquidator from claiming the not-auctioned liquidated NFT.

the low amount of payment 10001 WEI is not likely to cover the outstanding debt and the lender has to bear the loss.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol validate the caller of the safeTransferFrom in ClearingHouse is the seaport / conduict contract and check that when the auction is settled, the NFT ownership changed and the Astaria contract does not hold the NFT any more.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L605 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L672 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L808 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L766

Vulnerability details

Impact

LienToken payment can create more debt

Proof of Concept

function _payment in LienToken is used when a payment happens

function makePayment(
	uint256 collateralId,
	Stack[] calldata stack,
	uint8 position,
	uint256 amount
)
external
validateStack(collateralId, stack)
returns (Stack[] memory newStack)
{
LienStorage storage s = _loadLienStorageSlot();
(newStack, ) = _payment(s, stack, position, amount, msg.sender);
_updateCollateralStateHash(s, collateralId, newStack);
}

LienToken._payment use _getOwned to calculated the oustanding debt

uint256 owed = _getOwed(stack, block.timestamp);
address lienOwner = ownerOf(lienId);
bool isPublicVault = _isPublicVault(s, lienOwner);

address payee = _getPayee(s, lienId);

if (amount > owed) amount = owed;
if (isPublicVault) {
  IPublicVault(lienOwner).beforePayment(
	IPublicVault.BeforePaymentParams({
	  interestOwed: owed - stack.point.amount,
	  amount: stack.point.amount,
	  lienSlope: calculateSlope(stack)
	})
  );
}

//bring the point up to block.timestamp, compute the owed
stack.point.amount = owed.safeCastTo88();
stack.point.last = block.timestamp.safeCastTo40();

if (stack.point.amount > amount) {

note the function call

uint256 owed = _getOwed(stack, block.timestamp);

which calls:

/**
* @dev Computes the debt owed to a Lien at a specified timestamp.
* @param stack The specified Lien.
* @return The amount owed to the Lien at the specified timestamp.
*/
function _getOwed(Stack memory stack, uint256 timestamp)
internal
pure
returns (uint88)
{
return stack.point.amount + _getInterest(stack, timestamp).safeCastTo88();
}

the function call _getInterest can accumulate more interest and create more debt for user

/**
* @dev Computes the interest accrued for a lien since its last payment.
* @param stack The Lien for the loan to calculate interest for.
* @param timestamp The timestamp at which to compute interest for.
*/
function _getInterest(Stack memory stack, uint256 timestamp)
internal
pure
returns (uint256)
{
uint256 delta_t = timestamp - stack.point.last;

return (delta_t * stack.lien.details.rate).mulWadDown(stack.point.amount);
}

if he didn't pay all amount of lien, then next time he will pay more interests.

For example

User borrows 1 eth. His lien.amount is 1eth. Then he wants to repay some part(let's say 0.5 eth). Now his lien.amount becomes lien.amount + interests. When he pays next time, he pays (lien.amount + interests) + new interests. So interests are acummulated on previous interests.

Tools Used

Manual Reveiw

Recommended Mitigation Steps

Change the implementation so that LienToken payment bring down the debt for user.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/VaultImplementation.sol#L295 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L421 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L359 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L372 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L384

Vulnerability details

Impact

Certain function can be blocked if the ERC20 token revert in 0 amount transfer after PublicVault#transferWithdrawReserve is called

Proof of Concept

The function transferWithdrawReserve in Public Vault has no access control.

  function transferWithdrawReserve() public {
    VaultData storage s = _loadStorageSlot();

    if (s.currentEpoch == uint64(0)) {
      return;
    }

    address currentWithdrawProxy = s
      .epochData[s.currentEpoch - 1]
      .withdrawProxy;
    // prevents transfer to a non-existent WithdrawProxy
    // withdrawProxies are indexed by the epoch where they're deployed
    if (currentWithdrawProxy != address(0)) {
      uint256 withdrawBalance = ERC20(asset()).balanceOf(address(this));

      // prevent transfer of more assets then are available
      if (s.withdrawReserve <= withdrawBalance) {
        withdrawBalance = s.withdrawReserve;
        s.withdrawReserve = 0;
      } else {
        unchecked {
          s.withdrawReserve -= withdrawBalance.safeCastTo88();
        }
      }

      ERC20(asset()).safeTransfer(currentWithdrawProxy, withdrawBalance);
      WithdrawProxy(currentWithdrawProxy).increaseWithdrawReserveReceived(
        withdrawBalance
      );
      emit WithdrawReserveTransferred(withdrawBalance);
    }

    address withdrawProxy = s.epochData[s.currentEpoch].withdrawProxy;
    if (
      s.withdrawReserve > 0 &&
      timeToEpochEnd() == 0 &&
      withdrawProxy != address(0)
    ) {
      address currentWithdrawProxy = s
        .epochData[s.currentEpoch - 1]
        .withdrawProxy;
      uint256 drainBalance = WithdrawProxy(withdrawProxy).drain(
        s.withdrawReserve,
        s.epochData[s.currentEpoch - 1].withdrawProxy
      );
      unchecked {
        s.withdrawReserve -= drainBalance.safeCastTo88();
      }
      WithdrawProxy(currentWithdrawProxy).increaseWithdrawReserveReceived(
        drainBalance
      );
    }
  }

If this function is called, the token balance is transfered to withdrawProxy

uint256 withdrawBalance = ERC20(asset()).balanceOf(address(this));

and

ERC20(asset()).safeTransfer(currentWithdrawProxy, withdrawBalance);

However, according to

https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

Some tokens (e.g. LEND) revert when transfering a zero value amount.

If ERC20(asset()).balanceOf(address(this)) return 0, the transfer revert.

The impact is that transferWithdrawReserve is also used in the other place:

  function commitToLien(
    IAstariaRouter.Commitment calldata params,
    address receiver
  )
    external
    whenNotPaused
    returns (uint256 lienId, ILienToken.Stack[] memory stack, uint256 payout)
  {
    _beforeCommitToLien(params);
    uint256 slopeAddition;
    (lienId, stack, slopeAddition, payout) = _requestLienAndIssuePayout(
      params,
      receiver
    );
    _afterCommitToLien(
      stack[stack.length - 1].point.end,
      lienId,
      slopeAddition
    );
  }

which calls:

_beforeCommitToLien(params);

which calls:

  function _beforeCommitToLien(IAstariaRouter.Commitment calldata params)
    internal
    virtual
    override(VaultImplementation)
  {
    VaultData storage s = _loadStorageSlot();

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

which calls transferWithdrawReserve() which revet in 0 amount transfer.

Consider the case below:

1. User A calls commitToLien transaction is pending in mempool.
2. User B front-run User A's transaction by calling transferWithdrawReserve() and the PublicVault has no ERC20 token balance or User B just want to call transferWithdrawReserve and not try to front-run user A, but the impact and result is the same.
3. User B's transaction executes first,
4. User A first his transaction revert because the ERC20 token asset revert in 0 amount transfer in transferWithdrawReserve() call

uint256 withdrawBalance = ERC20(asset()).balanceOf(address(this));

// prevent transfer of more assets then are available
if (s.withdrawReserve <= withdrawBalance) {
withdrawBalance = s.withdrawReserve;
s.withdrawReserve = 0;
} else {
unchecked {
  s.withdrawReserve -= withdrawBalance.safeCastTo88();
}
}

ERC20(asset()).safeTransfer(currentWithdrawProxy, withdrawBalance);

This revertion not only impact commitToLien, but also impact PublicVault.sol#updateVaultAfterLiquidation

function updateVaultAfterLiquidation(
uint256 maxAuctionWindow,
AfterLiquidationParams calldata params
) public onlyLienToken returns (address withdrawProxyIfNearBoundary) {
VaultData storage s = _loadStorageSlot();

_accrue(s);
unchecked {
  _setSlope(s, s.slope - params.lienSlope.safeCastTo48());
}

if (s.currentEpoch != 0) {
  transferWithdrawReserve();
}
uint64 lienEpoch = getLienEpoch(params.lienEnd);
_decreaseEpochLienCount(s, lienEpoch);

uint256 timeToEnd = timeToEpochEnd(lienEpoch);
if (timeToEnd < maxAuctionWindow) {
  _deployWithdrawProxyIfNotDeployed(s, lienEpoch);
  withdrawProxyIfNearBoundary = s.epochData[lienEpoch].withdrawProxy;

  WithdrawProxy(withdrawProxyIfNearBoundary).handleNewLiquidation(
	params.newAmount,
	maxAuctionWindow
  );
}

transaction can revert in above code when calling

if (s.currentEpoch != 0) {
  transferWithdrawReserve();
}
uint64 lienEpoch = getLienEpoch(params.lienEnd);

if the address has no ERC20 token balance and the ERC20 token revert in 0 amount transfer after PublicVault#transferWithdrawReserve is called first

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol just return and do nothing when PublicVault#transferWithdrawReserve is called if the address has no ERC20 token balance.

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/TransferProxy.sol#L34 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L181 https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/LienToken.sol#L643

Vulnerability details

Impact

Lack of support for fee-on-transfer token.

Proof of Concept

In the codebase, the usage of safeTransfer and safeTransferFrom assume that the receiver receive the exact transferred amount.

src\AstariaRouter.sol:
  528      ERC20(IAstariaVaultBase(commitments[0].lienRequest.strategy.vault).asset())
  529:       .safeTransfer(msg.sender, totalBorrowed);
  530    }

src\ClearingHouse.sol:
  142  
  143:     ERC20(paymentToken).safeTransfer(
  144        s.auctionStack.liquidator,

  160      if (ERC20(paymentToken).balanceOf(address(this)) > 0) {
  161:       ERC20(paymentToken).safeTransfer(
  162          ASTARIA_ROUTER.COLLATERAL_TOKEN().ownerOf(collateralId),

src\PublicVault.sol:
  383  
  384:       ERC20(asset()).safeTransfer(currentWithdrawProxy, withdrawBalance);
  385        WithdrawProxy(currentWithdrawProxy).increaseWithdrawReserveReceived(

src\VaultImplementation.sol:
  393      payout = _handleProtocolFee(c.lienRequest.amount);
  394:     ERC20(asset()).safeTransfer(receiver, payout);
  395    }

  405        }
  406:       ERC20(asset()).safeTransfer(feeTo, fee);
  407      }

src\WithdrawProxy.sol:
  268      if (s.withdrawRatio == uint256(0)) {
  269:       ERC20(asset()).safeTransfer(VAULT(), balance);
  270      } else {

  280        if (balance > 0) {
  281:         ERC20(asset()).safeTransfer(VAULT(), balance);
  282        }

  297      }
  298:     ERC20(asset()).safeTransfer(withdrawProxy, amount);
  299      return amount;

However, according to https://github.com/d-xo/weird-erc20#fee-on-transfer

Some tokens take a transfer fee (e.g. STA, PAXG), some do not currently charge a fee but may do so in the future (e.g. USDT, USDC).

So the recipient address may not receive the full transfered amount, which can break the protocol's accounting and revert transaction.

The same safeTransfer and safeTransferFrom is used in the vault deposit / withdraw / mint / withdraw function.

Let us see a concrete example,

contract TransferProxy is Auth, ITransferProxy {
  using SafeTransferLib for ERC20;

  constructor(Authority _AUTHORITY) Auth(msg.sender, _AUTHORITY) {
    //only constructor we care about is  Auth
  }

  function tokenTransferFrom(
    address token,
    address from,
    address to,
    uint256 amount
  ) external requiresAuth {
    ERC20(token).safeTransferFrom(from, to, amount);
  }
}

the transfer Proxy also use

ERC20(token).safeTransferFrom(from, to, amount);

this transfer is used extensively

src\AstariaRouter.sol:
  208      RouterStorage storage s = _loadRouterSlot();
  209:     s.TRANSFER_PROXY.tokenTransferFrom(
  210        address(token),

src\LienToken.sol:
  184      );
  185:     s.TRANSFER_PROXY.tokenTransferFrom(
  186        params.encumber.stack[params.position].lien.token,

  654      if (payment > 0)
  655:       s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);
  656  

  860  
  861:     s.TRANSFER_PROXY.tokenTransferFrom(stack.lien.token, payer, payee, amount);
  862  

src\scripts\deployments\Deploy.sol:
  378        uint8(UserRoles.ASTARIA_ROUTER),
  379:       TRANSFER_PROXY.tokenTransferFrom.selector,
  380        true

  403        uint8(UserRoles.LIEN_TOKEN),
  404:       TRANSFER_PROXY.tokenTransferFrom.selector,
  405        true

If the token used charge transfer fee, the accounting below is broken:

When _payDebt is called

  function _payDebt(
    LienStorage storage s,
    address token,
    uint256 payment,
    address payer,
    AuctionStack[] memory stack
  ) internal returns (uint256 totalSpent) {
    uint256 i;
    for (; i < stack.length;) {
      uint256 spent;
      unchecked {
        spent = _paymentAH(s, token, stack, i, payment, payer);
        totalSpent += spent;
        payment -= spent;
        ++i;
      }
    }
  }

which calls _paymentAH

  function _paymentAH(
    LienStorage storage s,
    address token,
    AuctionStack[] memory stack,
    uint256 position,
    uint256 payment,
    address payer
  ) internal returns (uint256) {
    uint256 lienId = stack[position].lienId;
    uint256 end = stack[position].end;
    uint256 owing = stack[position].amountOwed;
    //checks the lien exists
    address payee = _getPayee(s, lienId);
    uint256 remaining = 0;
    if (owing > payment.safeCastTo88()) {
      remaining = owing - payment;
    } else {
      payment = owing;
    }
    if (payment > 0)
      s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);

    delete s.lienMeta[lienId]; //full delete
    delete stack[position];
    _burn(lienId);

    if (_isPublicVault(s, payee)) {
      IPublicVault(payee).updateAfterLiquidationPayment(
        IPublicVault.LiquidationPaymentParams({remaining: remaining})
      );
    }
    emit Payment(lienId, payment);
    return payment;
  }

not that the code

s.TRANSFER_PROXY.tokenTransferFrom(token, payer, payee, payment);

if the token charge transfer fee, for example, the payment amout is 100 ETH, 1 ETH is charged as fee, the recipient only receive 99 ETH,

but the wrong value payment 100 ETH is returned and used to update the accounting

unchecked {
	spent = _paymentAH(s, token, stack, i, payment, payer);
	totalSpent += spent;
	payment -= spent;
  ++i;
}

then the variable totalSpent and payment amoutn will be not valid.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol whitelist the token address or use balance before and after check to make sure the recipient receive the accurate amount of token when token transfer is performed.