`YearnVault.sol#pull()` will most certainly fail

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-12-mellow/blob/6679e2dd118b33481ee81ad013ece4ea723327b5/mellow-vaults/test_brownie/contracts/YearnVault.sol#L84-L101

    for (uint256 i = 0; i < _yTokens.length; i++) {
        if (tokenAmounts[i] == 0) {
            continue;
        }

        IYearnVault yToken = IYearnVault(_yTokens[i]);
        uint256 yTokenAmount = ((tokenAmounts[i] * (10**yToken.decimals())) / yToken.pricePerShare());
        uint256 balance = yToken.balanceOf(address(this));
        if (yTokenAmount > balance) {
            yTokenAmount = balance;
        }
        if (yTokenAmount == 0) {
            continue;
        }
        yToken.withdraw(yTokenAmount, to, maxLoss);
        (tokenAmounts[i], address(this));
    }
    actualTokenAmounts = tokenAmounts;

The actual token withdrew from yToken.withdraw() will most certainly be less than the tokenAmounts[i], due to precision loss in the calculation of yTokenAmount.

As a result, IERC20(_vaultTokens[i]).safeTransfer(to, actualTokenAmounts[i]); in LpIssuer.sol#withdraw() will revert due to insufficant balance.

Recommendation

Change to:

tokenAmounts[i] = yToken.withdraw(yTokenAmount, to, maxLoss);

[MihanixA]

Actually I don't see how this could lead to fund loss. I think this one is a bug. @0xleastwood what do you think?

[0xleastwood]

my understanding is that users won't be able to withdraw pushed funds @MihanixA

[0xleastwood]

so fund loss is related to not being able to withdraw rather than by extracting value from the protocol

[s0xn1ck]

While we agree that this will prevent full withdrawal of the funds, that wil be limited to only a couple of wei's which is the yearn precision loss. So in case you put 100eth you will be able to recover 100eth - 1wei. So we'd rather name the issue "some small amounts cannot be withdrawn from the pool"

[0xleastwood]

If my understanding is correct, YearnVault._pull will withdraw yTokenAmount representing the yToken's shares and then withdraw on this amount but return tokenAmounts where the amount withdrawn is typically less than the amount intended to be withdrawn. LpIssuer.withdraw() will expect actualTokenAmounts to be available to be transferred which isn't exactly in the contract's balance.

https://github.com/code-423n4/2021-12-mellow/blob/6679e2dd118b33481ee81ad013ece4ea723327b5/mellow-vaults/contracts/YearnVault.sol#L90 https://github.com/code-423n4/2021-12-mellow/blob/6679e2dd118b33481ee81ad013ece4ea723327b5/mellow-vaults/contracts/LpIssuer.sol#L152

[0xleastwood]

Let's use an example:

• Alice calls LpIssuer.withdraw() with tokensAmount[0] equal to 100 tokens. Let's ignore the lpTokenAmount argument for the sake of this example.
• _subvault().pull is called on this tokensAmount[0].
• yTokenAmount is calculated according to ((tokenAmounts[i] * (10**yToken.decimals())) / yToken.pricePerShare()); which potentially leads to a slightly truncated output.
• This truncated output represents the shares belonging to the user which is then parsed to yToken.withdraw().
• yToken.withdraw() is likely less than 100 tokens and is sent to the LpIssuer.sol contract but actualTokenAmounts[0] is equal to 100 tokens.
• LpIssuer.withdraw() attempts to send tokens to the withdrawer but is unable as the contract does not have sufficient balance. IERC20(_vaultTokens[i]).safeTransfer(to, actualTokenAmounts[i]);
• If I'm not mistaken, it seems like this issue would be apparent on any withdrawal amount (assuming there is some amount truncated).
• There is also an important edge case where the amount to withdraw from the yearn vault is greater than the available contract balance, it will always revert.

[s0xn1ck]

Agreed, thank you!