Inadequate checks on withdrawAVAX() and redeemAVAX() in TokenggAVAX.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/TokenggAVAX.sol#L180-L189
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/TokenggAVAX.sol#L191-L203

Vulnerability details

Impact

As denoted in ggAVAX via ERC4626:

"When a user redeems their ggAVAX for AVAX, there has to be funds available that are not being staked by the minipools.
If there is no AVAX that has been unstaked yet, the redemtion call will fail and the user will have to come back another time."

With protocolDAO.getTargetGGAVAXReserveRate() == 0.1 ether, i.e. a tenth of totalAssets(), it could impose a tight limit in situations when there is a rush for the liquid stakers to withdraw or redeem their deposited AVAX. When these were to happen, a great multitude of stakers would be disappointed with repeated function reverts mostly due to transactions being front run by others.

Proof of Concept

File: TokenggAVAX.sol#L180-L203

	function withdrawAVAX(uint256 assets) public returns (uint256 shares) {
		shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.
		beforeWithdraw(assets, shares);
		_burn(msg.sender, shares);

		emit Withdraw(msg.sender, msg.sender, msg.sender, assets, shares);

		IWAVAX(address(asset)).withdraw(assets);
		msg.sender.safeTransferETH(assets);
	}

	function redeemAVAX(uint256 shares) public returns (uint256 assets) {
		// Check for rounding error since we round down in previewRedeem.
		if ((assets = previewRedeem(shares)) == 0) {
			revert ZeroAssets();
		}
		beforeWithdraw(assets, shares);
		_burn(msg.sender, shares);

		emit Withdraw(msg.sender, msg.sender, msg.sender, assets, shares);

		IWAVAX(address(asset)).withdraw(assets);
		msg.sender.safeTransferETH(assets);
	}

As can be seen from the code block above, withdrawAVAX() and redeemAVAX() would only revert on the last code line of execution when the funds available dropped below assets.

This could have been maximally mitigated from the beginning of the function logic by incorporating maxWithdraw() and maxRedeem() that had been implemented in the same contract, TokenggAVAX.sol.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider having withrawAVAX() and redeemAVAX() refactored as follows:

	function withdrawAVAX(uint256 assets) public returns (uint256 shares) {
+		uint256 assets_ = maxWithdraw(msg.sender);
+		if (assets_ < assets) {
+                       assets = assets_;
+               }
		shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.
		beforeWithdraw(assets, shares);
		_burn(msg.sender, shares);

                ...

	function redeemAVAX(uint256 shares) public returns (uint256 assets) {
+		uint256 shares_ = maxRedeem(msg.sender);
+		if (shares_ < shares) {
+                       shares = shares_;
+               }
		// Check for rounding error since we round down in previewRedeem.
		if ((assets = previewRedeem(shares)) == 0) {
			revert ZeroAssets();
		}
		beforeWithdraw(assets, shares);
		_burn(msg.sender, shares);

                ...

[c4-judge]

GalloDaSballo marked the issue as duplicate of #417

[c4-judge]

GalloDaSballo marked the issue as not a duplicate

[GalloDaSballo]

L

[c4-judge]

Duplicate of #65