cause fund loss in TokenggAVAX by early direct fund transfer and manipulating price per share

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L42-L54
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L132-L134
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L120-L124
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L113-L130
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L88-L109

Vulnerability details

Impact

by performing this attack, attacker can cause other users to lose their funds when they are depositing their AVAX tokens into TokenggAVAX pool. when ever users deposits AVAX token into TokenggAVAX, contract calculates shares amount based on percentage of the AVAX received amount to total AVAX balance of the contract. attacker can make the ratio AVAX to share very high (like 100 * 1e18) by early direct AVAX transfer to contract address and this would cause very high division error in convertToShares() function and users would lose their funds up to ratio when depositing or withdrawing their funds.

Proof of Concept

This is convertToShares() code which is used to calculate shares amount during the user's deposits:

	function convertToShares(uint256 assets) public view virtual returns (uint256) {
		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

		return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
	}

As you can see if the totalAssets() where so much bigger than supply then the division error would be very high for shares amount calculation (up to totalAssets() / supply) and user would receive shares that their values are much less than AVAX tokens he deposited. attacker can cause totalAsset() / supply ratio to be very high (up to 1000 * 1e18) and perform this attack. again when withdrawing contract uses function previewWithdraw():

	function previewWithdraw(uint256 assets) public view virtual returns (uint256) {
		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

		return supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());
	}

and if attacker performs the attack and create big totalAssets() / supply ratio then user's more shares would get burned each time he withdraws because of the rounding up and division error.
To make a big totalAssets() / supply ratio attacker needs to transfer AVAX tokens (for example 1000 * 1e18) directly to the contract address when contract deployed recently and has no or low deposits, and call the syncRewards() function so those transferred tokens get considered as rewards (when block.timestamp is close to nextRewardsCycleEnd). then attacker needs to call deposit() and deposit 1 wei AVAX and mint 1 share. so total supply would be 1 and totalAssets() would be 1000 * 1e18 (after reward duration ends) and totalAssets() / supply = 1000 * 1e18.
these are the steps attacker need to perform:

1. wait for TokenggAVAX to get deployed.
2. wait for block.timestamp to be near rewardsCycleLength multiple. (for example there is 1000 seconds for next cycle begin time).
3. in on transaction send 1000 * 1e18 AVAX tokens to the contract and call syncRewards() and contract would consider the 1000 * 1e18 amount as rewards and start increasing totalAssets() linearly up to 1000 * 1e18 until end of the cycle(in 1000 seconds).
4. and also deposit 1 wei AVAX into the contract and receive 1 share.
5. 1 seconds after this deposit, the totalAssets() would be about 1e18 (1000 seconds until end of the cycle and in each seconds contract would release 1e18 tokens) and supply would be 1 and every user depositing or withdrawing would lose up to 1e18 tokens because of the rounding error in division. in this second if users deposits 15 * 1e17 AVAX tokens then contract would mint 1 share for him which worth 1e18 AVAX and users loses 5 * 1e17 of his token in his deposits. and if user withdraws 1 wei AVAX tokens contract would burn his 1 shares and users would lose the rest of his funds,.
6. at end of the cycle the ratio of the totalAssets() / supply would be 1000 * 1e18 and users would lose up to 1000 * 1e18.

the success and impact of this attack depends on closeness of block.timestamp to the rewardsCycleLength multiple (closer to the multiple the more fund loss) and amount of AVAX attacker transfers directly into the contract address. in general attacker can create high PPS ratio (the exact value of PPS that attacker can create may be differ based on the situation). the point is PPS would not get decreased after attack and users would lose funds afterwards and contract's broken state is not recoverable.

Tools Used

VIM

Recommended Mitigation Steps

check for minimum deposit amount or add extra decimals for share amount.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #209

[c4-judge]

GalloDaSballo marked the issue as satisfactory