code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-goodentry/blob/main/contracts/GeVault.sol#L247-L284

Vulnerability details

Impact

A well known attack vector for almost all shares based liquidity pool contracts, where an early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share. This vulnerability is mentioned in a previous C4 contest here, and in this Trail of Bits audit (issue 003).

Vulnerability Detail:

The attack works in the following way:

A malicious early user can deposit() with 1 wei of asset token as the first depositor of the LToken, and get 1 wei of shares.
Then the attacker can send 10000e18 - 1 of asset 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 shares token.
They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit().

For a more detailed description, refer to this one from OpenZeppelin.

Proof of Concept

File: contracts/GeVault.sol

247:   function deposit(address token, uint amount) public payable nonReentrant returns (uint liquidity) 
248:   {
249:     require(isEnabled, "GEV: Pool Disabled");
250:     require(poolMatchesOracle(), "GEV: Oracle Error");
251:     require(token == address(token0) || token == address(token1), "GEV: Invalid Token");
252:     require(amount > 0 || msg.value > 0, "GEV: Deposit Zero");
253:     
254:     // Wrap if necessary and deposit here
255:     if (msg.value > 0){
256:       require(token == address(WETH), "GEV: Invalid Weth");
257:       // wraps ETH by sending to the wrapper that sends back WETH
258:       WETH.deposit{value: msg.value}();
259:       amount = msg.value;
260:     }
261:     else { 
262:       ERC20(token).safeTransferFrom(msg.sender, address(this), amount);
263:     }
264:     
265:     // Send deposit fee to treasury
266:     uint fee = amount * getAdjustedBaseFee(token == address(token0)) / 1e4;
267:     ERC20(token).safeTransfer(treasury, fee);
268:     uint valueX8 = oracle.getAssetPrice(token) * (amount - fee) / 10**ERC20(token).decimals();
269:     require(tvlCap > valueX8 + getTVL(), "GEV: Max Cap Reached");
270: 
271:     uint vaultValueX8 = getTVL();
272:     uint tSupply = totalSupply();
273:     // initial liquidity at 1e18 token ~ $1
274:     if (tSupply == 0 || vaultValueX8 == 0)
275:       liquidity = valueX8 * 1e10;
276:     else {
277:       liquidity = tSupply * valueX8 / vaultValueX8;
278:     }
279:     
280:     rebalance();
281:     require(liquidity > 0, "GEV: No Liquidity Added");
282:     _mint(msg.sender, liquidity);    
283:     emit Deposit(msg.sender, token, amount, liquidity);

Tools Used

Manual review

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a portion of the initial mints as a reserve to the DAO/ burn so that the price per share can be more resistant to manipulation. Refer to OpenZeppelin's ERC4626 implementation for more details on the vulnerability and a coded example of a workaround.

Assessed type

ERC4626

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #367

c4-judge commented 1 year ago

gzeon-c4 changed the severity to 2 (Med Risk)