Depositing Underlying Assets is susceptible to ERC4626 Inflation Attacks

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L183

Vulnerability details

Impact

IBT may well be a good trusted vault due to protocol or using the ERC4626 standard from OpenZeppelin. However, despite all the good intentions, it may be problematic as it may fail to protect against Inflation Attacks First Depositor Attacks by using older OpenZeppelin versions of ERC4626 contracts that do not offer offset mitigations. Alternatively, it may be a good implementation of ERC4626 oblivious to the inflation attacks in the share calculations.

Attackers can manipulate the value of shares in ERC4626 and reduce or result in zero shares of ibts returned from the ERC4626 vault and therefore users depositing zero or small amounts of ibts into this protocol disadvantaging them to get lower or no yields than expected leading to losses of returns or the underlying assets they deposited.

Inflation attack may also lead to DOS on the protocol in the case where users opt to use function with slippage [minShares] if vault has been first depositor attacked most expected minShares from users will never be obtained so protocol becomes unusable through this function below

function deposit(
        uint256 assets,
        address ptReceiver,
        address ytReceiver,
        uint256 minShares
    )

Proof of Concept

function deposit(
        uint256 assets,
        address ptReceiver,
        address ytReceiver
    ) public override returns (uint256 shares) {
        IERC20(_asset).safeTransferFrom(msg.sender, address(this), assets);
        IERC20(_asset).safeIncreaseAllowance(ibt, assets);
        uint256 ibts = IERC4626(ibt).deposit(assets, address(this));
        shares = _depositIBT(ibts, ptReceiver, ytReceiver);
    }

The deposit function above has no way to enforce that the IERC4626 has any protection against Inflation Attacks. For example in the vault of ibts any user can transfer token to vault to inflate totalSupply and ensure can deposit 1 share set totalSupply = 1. And transfer token to vault to inflate totalAssets() and force all subsequent deposits to use this share price as base resulting in zero or little shares returned to other users.

Tools Used

Manual Analysis

Recommended Mitigation Steps

It is recommended the normal deposit function be removed so only deposit function with slippage minShares is left to be used by users It is recommended to maybe ensure only ibts that are used in this protocol are from vault that has Inflation Attack Mitigation It is recommended to communicate this to all user the potential of this attack

Assessed type

ERC4626

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #47

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid