Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First depositor can break the minting of shares #736

Closed
code423n4 opened this issue Jan 3, 2023 · 2 comments
Closed

First depositor can break the minting of shares #736

code423n4 opened this issue Jan 3, 2023 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-209 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

An early depositor can break the future minting of shares by minting a very small number of shares, but donating a large amount of AVAX during the reward period, such that each wei of ggAVAX is worth a large amount.

Impact

If one wei of ggAVAX is worth, say, 1 Eth of AVAX, if a user calls deposit() with a value less than this amount, the amount the number of shares they get back round down to zero due to loss of precision and the call will revert, essentially bricking the contract for users with small amounts. If a user instead deposits an amount larger than 1 Eth, the amount over 1 Eth is given to all shareholders due to loss of precision.

Proof of Concept

An attacker can deposit 1 wei of AVAX, then transfer a large amount of AVAX to the TokenggAVAX contract via a selfdestruct(), then call syncRewards(). That function will convert the balance of the contract to lastRewardsAmt...:

// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.syncRewards()   #1
99  @> 		uint256 nextRewardsAmt = (asset.balanceOf(address(this)) + stakingTotalAssets_) - totalReleasedAssets_ - lastRewardsAmt_;
100    
101    		// Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`.
102    		uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
103    
104:@> 		lastRewardsAmt = nextRewardsAmt.safeCastTo192();

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

...which is used in totalAssets() either in the next period, or proportionally in the current period, depending on how much time has elapsed...:

// File: contracts/contract/tokens/TokenggAVAX.sol : TokenggAVAX.totalAssets()   #2

120    		if (block.timestamp >= rewardsCycleEnd_) {
121    			// no rewards or rewards are fully unlocked
122    			// entire reward amount is available
123 @> 			return totalReleasedAssets_ + lastRewardsAmt_;
124    		}
125    
126    		// rewards are not fully unlocked
127    		// return unlocked rewards and stored total
128 @> 		uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
129    		return totalReleasedAssets_ + unlockedRewards;
130:   	}

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

...and that total is used to determine the worth of every share during minting:

// File: contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol : ERC4626Upgradeable.convertToShares()   #3

120    	function convertToShares(uint256 assets) public view virtual returns (uint256) {
121    		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
122    
123 @> 		return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
124:   	}

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L120-L124

Tools Used

Code inspection

Recommended Mitigation Steps

Upon initialization, mint an initial amount of shares to an address that is not able to withdraw them

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jan 3, 2023
code423n4 added a commit that referenced this issue Jan 3, 2023
C4-Staff added a commit that referenced this issue Jan 6, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Jan 8, 2023

GalloDaSballo marked the issue as duplicate of #209

@c4-judge
Copy link
Contributor

c4-judge commented Feb 8, 2023

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-209 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants