Astaria contest - Apocalypto's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L70-L73

Vulnerability details

Impact

According to the documentation, Private Vaults do not interact with AstariaRouter except during deployment. Authorized users can deposit tokens into private vaults to fund strategies, but the withdraw function uses safeTransferFrom without first calling approve which means that deposited tokens will be permanently locked.

Proof of Concept

The Vault.withdraw function uses safeTransferFrom from the solmate ERC20 library to send tokens back to their owners. This function then calls the transferFrom function internally.

    function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public virtual returns (bool) {
        uint256 allowed = allowance[from][msg.sender]; // Saves gas for limited approvals.

        if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amount;

As shown in the code, the function Vault.withdraw requires a preceding call to approve as it calculates the allowance based on it. Since private Vaults do not interact with AstariaRouter, which uses safeApprove for allowance in Public Vaults, the "transferFrom function always reverts due to underflow, leaving the tokens stuck in the Vault.

This PoC simulate the bug:

pragma solidity =0.8.17;

import "forge-std/Test.sol";

import {Authority} from "solmate/auth/Auth.sol";
import {FixedPointMathLib} from "solmate/utils/FixedPointMathLib.sol";
import {MockERC721} from "solmate/test/utils/mocks/MockERC721.sol";
import {
  MultiRolesAuthority
} from "solmate/auth/authorities/MultiRolesAuthority.sol";

import {ERC721} from "gpl/ERC721.sol";
import {SafeCastLib} from "gpl/utils/SafeCastLib.sol";

import {IAstariaRouter, AstariaRouter} from "../AstariaRouter.sol";
import {VaultImplementation} from "../VaultImplementation.sol";
import {PublicVault} from "../PublicVault.sol";
import {Vault} from "../Vault.sol";
import {TransferProxy} from "../TransferProxy.sol";
import {WithdrawProxy} from "../WithdrawProxy.sol";

import {Strings2} from "./utils/Strings2.sol";

import "./TestHelpers.t.sol";
import {OrderParameters} from "seaport/lib/ConsiderationStructs.sol";

contract AstariaTest is TestHelpers {
  using FixedPointMathLib for uint256;
  using CollateralLookup for address;
  using SafeCastLib for uint256;

  event NonceUpdated(uint256 nonce);
  event VaultShutdown();

  function testVaultWithdrawDoS() external {
    TestNFT nft = new TestNFT(2);
    address tokenContract = address(nft);
    uint256 tokenId = uint256(1);

    uint256 initialBalance = WETH9.balanceOf(address(this));

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

    _lendToPrivateVault(
      Lender({addr: strategistOne, amountToLend: 50 ether}),
      privateVault
    );

    vm.startPrank(strategistOne);

    vm.expectRevert("TRANSFER_FROM_FAILED");
    Vault(privateVault).withdraw(1 ether);

  }
}

Tools Used

Manual aditing, foundry

Recommended Mitigation Steps

Instead of safeTransferFrom it is recommended to use safeTransfer.