`YearnAdapter._yTotalAssets` function uses `IERC20(asset()).balanceOf(address(yVault))` instead of Yearn vault's `self.totalIdle`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L80-L82 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L89-L98 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L101-L103 https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L109-L111 https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L809-L813 https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L1811-L1838

Vulnerability details

Impact

The YearnAdapter._yTotalAssets function below attempts to resemble Yearn's Vault._totalAssets function but uses IERC20(asset()).balanceOf(address(yVault)) as part of the total quantity of all assets under control of the Yearn vault. In contrast, Yearn's Vault._totalAssets function below uses the self.totalIdle accounting variable instead of the asset balance owned by the vault returned by the asset token's balanceOf function. As shown by Yearn's Vault.sweep function below, it is possible that extra asset token amount is sent to the Yearn vault by accident in which the governance can withdraw such extra amount from the vault.

When such extra asset token amount is transferred to the Yearn vault, the YearnAdapter._yTotalAssets function will return a value that is bigger than it should be because IERC20(asset()).balanceOf(address(yVault)) is bigger than the Yearn vault's self.totalIdle at that moment. This will cause YearnAdapter._totalAssets() to be higher than it should be, which will inflate AdapterBase.totalAssets() and Vault.totalAssets() for the vault that uses the YearnAdapter contract as its adapter. Hence, comparing to if Yearn vault's self.totalIdle is used in the YearnAdapter._yTotalAssets function, calling the Vault.deposit function will mint less shares for depositing a given asset amount, and calling the Vault.mint function will deposit more asset amount for minting a given number of shares. If Yearn vault's governance has withdrawn the extra asset token amount before the Vault.withdraw or Vault.redeem function is called, calling the Vault.withdraw or Vault.redeem function will transfer an asset amount that is less than the deposited amount to the user in this case. As a result, the user loses the difference between the deposited and withdrawn asset amounts.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L80-L82

    function _totalAssets() internal view override returns (uint256) {
        return _shareValue(underlyingBalance);
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L89-L98

    function _shareValue(uint256 yShares) internal view returns (uint256) {
        if (yVault.totalSupply() == 0) return yShares;

        return
            yShares.mulDiv(
                _freeFunds(),
                yVault.totalSupply(),
                Math.Rounding.Down
            );
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L101-L103

    function _freeFunds() internal view returns (uint256) {
        return _yTotalAssets() - _calculateLockedProfit();
    }

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/adapter/yearn/YearnAdapter.sol#L109-L111

    function _yTotalAssets() internal view returns (uint256) {
        return IERC20(asset()).balanceOf(address(yVault)) + yVault.totalDebt();
    }

https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L809-L813

@view
@internal
def _totalAssets() -> uint256:
    # See note on `totalAssets()`.
    return self.totalIdle + self.totalDebt

https://github.com/yearn/yearn-vaults/blob/master/contracts/Vault.vy#L1811-L1838

@external
def sweep(token: address, amount: uint256 = MAX_UINT256):
    ...
    assert msg.sender == self.governance
    # Can't be used to steal what this Vault is protecting
    value: uint256 = amount
    if value == MAX_UINT256:
        value = ERC20(token).balanceOf(self)

    if token == self.token.address:
        value = self.token.balanceOf(self) - self.totalIdle

    log Sweep(token, value)
    self.erc20_safe_transfer(token, self.governance, value)

Proof of Concept

The following steps can occur for the described scenario.

1. A vault that uses the YearnAdapter contract as its adapter exists.
2. An extra asset token amount is sent to the used Yearn vault by accident so IERC20(asset()).balanceOf(address(yVault)) is now bigger than the Yearn vault's self.totalIdle.
3. A user calls the Vault.mint function to mint 1 share, which deposits 10e18 asset tokens to the Yearn vault.
4. The Yearn vault's governance calls the Yearn vault's sweep function to withdraw the extra asset token amount from the Yearn vault.
5. The user calls the Vault.redeem function to redeem 1 share but only 9e18 asset tokens are withdrawn. Hence, the user loses 1e18 asset tokens.

Tools Used

VSCode

Recommended Mitigation Steps

The YearnAdapter._yTotalAssets function can be updated to use the Yearn vault's self.totalIdle instead of IERC20(asset()).balanceOf(address(yVault)).

[c4-judge]

dmvt marked the issue as primary issue

[RedVeil]

Looking at a current vault like https://etherscan.io/address/0x27B5739e22ad9033bcBf192059122d163b60349D We see that totalAssets gets calculated like this:

def _totalAssets() -> uint256:
    # See note on `totalAssets()`.
    return self.token.balanceOf(self) + self.totalDebt

Which matches the implementation

[c4-sponsor]

RedVeil marked the issue as sponsor disputed

[c4-sponsor]

RedVeil marked the issue as disagree with severity

[c4-judge]

dmvt marked issue #728 as primary and marked this issue as a duplicate of 728

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as partial-50

[rbserver]

Hi @dmvt,

Comparing to #728, this report and #728 both describe the scenario that the return value of the YearnAdapter._yTotalAssets function can be inflated because Yearn vault's self.totalIdle accounting variable is not used. Both reports also mention about Yearn's Vault.sweep function that can be performed by Yearn's governance. Thus, the information and content of both reports are very similar. Yet, this report is rated a partial-50 instead of a full score. Hence, I would like to ask about the reason behind this.

Thanks again for your work and time!

[c4-judge]

dmvt marked the issue as full credit

[c4-judge]

dmvt marked the issue as satisfactory

[dmvt]

Hi @dmvt,

Comparing to #728, this report and #728 both describe the scenario that the return value of the YearnAdapter._yTotalAssets function can be inflated because Yearn vault's self.totalIdle accounting variable is not used. Both reports also mention about Yearn's Vault.sweep function that can be performed by Yearn's governance. Thus, the information and content of both reports are very similar. Yet, this report is rated a partial-50 instead of a full score. Hence, I would like to ask about the reason behind this.

Thanks again for your work and time!

You're correct. My mistake. Full credit restored.