Astaria contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/main/src/VaultImplementation.sol#L239

Vulnerability details

Impact

An unapproved, non-owner of collateral can still take loan for the owner/operator of collateral even when owner did not needed any loan. This is happening due to incorrect checks as shown in POC. This leads to unintended loan and associated fees for users

Proof of Concept

1. A new loan is originated via commitToLien function by User X. Params used by User X are as below:

collateralId = params.tokenContract.computeId(params.tokenId) = 1

CT.ownerOf(1) = User Y

CT.getApproved(1) = User Z

CT.isApprovedForAll(User Y, User X) = false

receiver = User Y

2. This internally make call to _requestLienAndIssuePayout which then calls _validateCommitment function for signature verification
3. Lets see the signature verification part in _validateCommitment function

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

4. Ideally the verification should fail since :

a. User X is not owner of passed collateral b. User X is not approved for this collateral c. User X is not approved for all of User Y token

5. But observe the below if condition doing the required check:

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

6. In our case this if condition does not execute since receiver = holder

if (
      msg.sender != holder && // true since User X is not the owner
      receiver != holder && // false since attacker passed receiver as User Y which is owner of collateral, thus failing this if condition 
      receiver != operator &&
      !CT.isApprovedForAll(holder, msg.sender)
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }

7. This means the signature verification passes and loan is issued for collateral owner without his wish

Recommended Mitigation Steps

Revise the condition as shown below:

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

if (
      receiver != holder &&
      receiver != operator 
    ) {
      revert InvalidRequest(InvalidRequestReason.NO_AUTHORITY);
    }

Lines of code

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

Vulnerability details

Impact

The condition verifying that deposited amount is greater than minDepositAmount() is incorrect. Instead of checking assets > minDepositAmount(), the function incorrectly checks shares > minDepositAmount(). This could either allow :

1. User to deposit amount lesser than minDepositAmount - if share allotted per asset is large
2. Genuine deposit to fail - if share allotted per asset is low

Proof of Concept

1. Assume minDepositAmount() is 100
2. User A calls the deposit function with amount 1000

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

3. lets say this resulted in 5 shares from previewDeposit(assets)

4. The deposit fails since 5<minDepositAmount()=100

5. But this is incorrect since user deposit amount was 1000 which is 10x more than min amount

Recommended Mitigation Steps

Revise the deposit function as below (this is done correctly in mint function)

function deposit(uint256 assets, address receiver)
    public
    virtual
    returns (uint256 shares)
  {
...
    require(assets > minDepositAmount(), "VALUE_TOO_SMALL");
    
...
  }

Lines of code

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

Vulnerability details

Impact

It is possible to make a deposit even when _loadVISlot().isShutdown is true. This check is done in whenNotPaused modifier and is already done for Public Vault but is missing for Private Vault, allowing unexpected deposits

Proof of Concept

1. Observe the deposit function at https://github.com/code-423n4/2023-01-astaria/blob/main/src/Vault.sol#L59

function deposit(uint256 amount, address receiver)
    public
    virtual
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
    require(s.allowList[msg.sender] && receiver == owner());
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
    return amount;
  }

2. Observe there is no whenNotPaused modifier in deposit which confirm that shutdown has not been called yet

modifier whenNotPaused() {
    if (ROUTER().paused()) {
      revert InvalidRequest(InvalidRequestReason.PAUSED);
    }

    if (_loadVISlot().isShutdown) {
      revert InvalidRequest(InvalidRequestReason.SHUTDOWN);
    }
    _;
  }

Recommended Mitigation Steps

Revise the deposit function as below:

function deposit(uint256 amount, address receiver)
    public
    virtual 
    whenNotPaused
    returns (uint256)
  {
    VIData storage s = _loadVISlot();
    require(s.allowList[msg.sender] && receiver == owner());
    ERC20(asset()).safeTransferFrom(msg.sender, address(this), amount);
    return amount;
  }