GoGoPool contest - Arbor-Finance's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L24

Vulnerability details

Impact

Empty ERC4626 vaults can be manipulated to inflate the price of a share and cause depositors to lose their deposits due to rounding in favor of the vault. In particular, this can be done as a frontrunning attack if the vault is used without specifying a minimum of shares to be received using an ERC4626 router.

The inflation attack comes from the amount of underlying AVAX changing without the amount of ggAVAX reflecting that. This inflation makes all the ggAVAX more valuable, which means that someone buying ggAVAX will get less than they would have expected.

The bigger the inflation, the bigger the effect of the attack, which is why this is particularly sensitive when then vault is its early stages, with low liquidity.

Proof of Concept

A malicious early user can deposit() with 1 wei of AVAX as the first depositor of the Vault, and get 1 wei of ggAVAX.

Then the attacker can send 10000e18-1 of AVAX tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 (from 1+10000e18-1)/1).

As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of ggAVAX.

They will immediately lose 9999e18 or half of their deposits if they redeem right after the deposit()

Tools Used

Visual Studio Code

Recommended Mitigation Steps

---

1. Using an ERC4626 router
2. Keeping track of the total assets internally
3. Creating "dead shares"

---

The first solution is the most simple one, but also possibly the less satisfying. It doesn't actually address the issue as much as it goes around it.

Advantage:

• Already available
• Easy to reason by
• Nothing to change to the vault's code

Drawback:

• Uses more gas (router needs to take custody before performing vault operations)
• Additional trust assumption (the router is a critical element that will be allowed to spend user token)
• User are still subject to slippage (they can limit it, but there is still room for "MEV")

---

Keeping track of the assets held by the vault internally remove the effect of direct transfers. Tokens transferred directly are not accounted for (unlike tokens that are explicitly transferred during a mint/deposit operation), which completely removes the risk of inflation attack.

This however causes other issues. Transferring tokens is still possible, and if they are not accounted for automatically, they might not be recoverable. To recover them would likely require some form of access control, which might not be desirable when building trust minimizing applications.

In addition to direct transfers, this also apply to re-basing tokens!

Advantage:

• Completely removes the inflation, removing all slippage
• Easy to reason by
• Self contained to the vault (no dependency on third party contract)
• Same workflow for EOAs and smart contracts
• This solution can provided as an optional module, that devs would add on top of the base implementation.

Drawback:

• Not generic (does not work with re-basing tokens)
• Risk of token lock (burn) OR needs a trusted admin
• Still some gas cost overhead (but less than the previous option)

---

The last option is inspired by Uniswap V2, which created some "dead LP shares" when the first liquidity is deposited. That basically means that the first liquidity provider is giving away a fraction of its shares for monetary stability.

A similar approach could be implemented in ERC4626 vaults, by minting "dead shares" on the first deposit/mint. For any assets that are then transferred to the vault, a fraction would be associated to these dead shares, and not redeemable by anyone.

In addition to not solving the actual problem (it is still possible to do inflation attacks, the profit is just reduced) this approach disincentivize any early liquidity providing that is not part of an inflation attack. It asks a lot of questions with the amount of "dead shares" to mint that possibly don't have an absolute good answer, and must be answered on a case-by-case basis.

Advantage:

• Very limited gas overhead.
• This solution can provided as an optional module, that devs would add on top of the base implementation.

Drawback:

• Doesn't truly resolve the issue
• Disincentivize legitimate early liquidity providers
• Locks a fraction of the assets in the vault (that can never be withdrawn)

---

Mitigation steps posted by @Amxx in https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3706#issuecomment-1297199749