cuthalion0x - First position opened in `AuraSpell` creates a debt that cannot be repaid

[sherlock-admin]

cuthalion0x

high

First position opened in AuraSpell creates a debt that cannot be repaid

Summary

The first call to AuraSpell.openPositionFarm will borrow tokens but never deposit them into the Balancer pool. The borrowed tokens are then left in the AuraSpell contract, and the user has acquired a debt with no benefit, thereby losing funds.

Vulnerability Detail

In AuraSpell.openPositionFarm, we borrow a single token to deposit into the Balancer pool. However, we then check the AuraSpell's balances of both the pool's tokens and use the minimum balance to compute poolAmountOut. If poolAmountOut is zero, we skip the BalancerVault.joinPool step.

Because we only borrowed one token in the first place, it is inevitable that the other token's balance will be zero. So, poolAmountOut is also zero, and we are guaranteed not to join the pool. If a second user then calls AuraSpell.openPositionFarm and chooses to borrow the other token, both balances will then be positive, and the pool can be joined. The second user has effectively stolen the first user's borrowed tokens.

However, the join is also not properly encoded (see #2), so the scenario involving the second user is only hypothetical. If the join encoding is not fixed, then the first user simply borrows tokens and leaves them stuck in the AuraSpell contract. It is still a loss of funds for the first user, who acquires a debt for no benefit.

Note that this also applies not only to the very first call, but also all subsequent calls utilizing the same param.borrowToken as the first. The issue can continue in perpetuity, but it is guaranteed to impact at least the first call.

Impact

A user borrows tokens and leaves them stuck, creating a debt that can never be repaid.

Code Snippet

spell/AuraSpell.sol#L86-L121, annotated below for clarity.

// 3. Add liquidity on Balancer, get BPT
{
    IBalancerVault vault = wAuraPools.getVault(lpToken);
    _ensureApprove(param.borrowToken, address(vault), borrowBalance);

    (address[] memory tokens, uint256[] memory balances, ) = wAuraPools
        .getPoolTokens(lpToken);
    uint[] memory maxAmountsIn = new uint[](2);
    // @audit-info We only borrowed at most one of these tokens (`param.borrowToken`), so the other balance will be zero.
    maxAmountsIn[0] = IERC20(tokens[0]).balanceOf(address(this));
    maxAmountsIn[1] = IERC20(tokens[1]).balanceOf(address(this));

    uint totalLPSupply = IBalancerPool(lpToken).totalSupply();
    // @audit-info One of these will be zero too.
    // compute in reverse order of how Balancer's `joinPool` computes tokenAmountIn
    uint poolAmountFromA = (maxAmountsIn[0] * totalLPSupply) /
        balances[0];
    uint poolAmountFromB = (maxAmountsIn[1] * totalLPSupply) /
        balances[1];

    // @audit-info This takes the minimum of A and B. Since one of them will be zero, `poolAmountOut` will also be zero.
    uint poolAmountOut = poolAmountFromA > poolAmountFromB
        ? poolAmountFromB
        : poolAmountFromA;

    bytes32 poolId = bytes32(param.farmingPoolId);
    // @audit-info If `poolAmountOut` is always zero, then this check always fails, and we never join the pool.
    if (poolAmountOut > 0) {
        vault.joinPool(
            poolId,
            address(this),
            address(this),
            IBalancerVault.JoinPoolRequest(
                tokens,
                maxAmountsIn,
                "",
                false
            )
        );
    }
}
// @audit-info If we get this far without having joined the pool, then any borrowed tokens are still sitting in this contract,
// and the caller will have acquired debt for no benefit. This effectively represents a loss of funds.

Tool used

Manual Review

Recommendation

First, create an integration test for the Aura spell. There are tests for Convex, Curve, and Ichi, but not Aura. A test would likely have caught this issue.

Reframe the logic around computing the deposit amount. It should only consider a single token (param.borrowToken) and not both, since it is not possible to borrow two tokens simultaneously. The IchiSpell approaches this more reasonably and could serve as inspiration.

Duplicate of #46