GoGoPool contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

If a malicious user is the first person who deposits AVAX in TokenggAVAX, he can prevent other users from depositing by applying grieving attack.

Proof of Concept

• Suppose Bob (a malicious user) is the first person who is interacting with the the contract ToeknggAVAX.
• He deposits 1 AVAX by calling the function depositAVAX.

function depositAVAX() public payable returns (uint256 shares) {
		uint256 assets = msg.value;
		// Check for rounding error since we round down in previewDeposit.
		if ((shares = previewDeposit(assets)) == 0) {
			revert ZeroShares();
		}

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

		IWAVAX(address(asset)).deposit{value: assets}();
		_mint(msg.sender, shares);
		afterDeposit(assets, shares);
	}

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

• So, one ggAVAX will be minted to Bob. The state will be: totalSupply = 1 and totalAssets() = 1. Please note that totalAssets is increased by one because totalReleasedAssets is increased in afterDeposit.

function previewDeposit(uint256 assets) public view virtual returns (uint256) {
        return convertToShares(assets);
    }

https://github.com/code-423n4/2022-12-gogopool/blob/1c30b320b7105e57c92232408bc795b6d2dfa208/lib/solmate/src/mixins/ERC4626.sol#L136

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());
    }

https://github.com/code-423n4/2022-12-gogopool/blob/1c30b320b7105e57c92232408bc795b6d2dfa208/lib/solmate/src/mixins/ERC4626.sol#L124

function afterDeposit(
		uint256 amount,
		uint256 /* shares */
	) internal override {
		totalReleasedAssets += amount;
	}

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

• Now Alice (an honest user) would like to deposit 100 AVAX.
• Bob notices Alice's transaction on the Mempool and applies front-run attack, and directly (not through calling depositAVAX) transfers 100 AVAX to the contract TokenggAVAX. So the state will be: totalSupply = 1 and totalAssets() = 101
• Now Alice's transaction is going to be executed. In the calculation of the shares, we have 100 * 1 /(101) = 0. So Alice's transaction will be failed due to ZeroShares error.

if ((shares = previewDeposit(assets)) == 0) {
			revert ZeroShares();
		}

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

• Bob could prevent Alice from depositing.
• Bob now can withdraw his 101 AVAX by calling the function withdrawAVAX(101). By doing so, the state will be: totalSupply = 0 and totalAssets() = 0.

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);
	}

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

In summary, Bob can apply this grieving attack to all the users who are going to deposit, and preventing them from depositing in the protocol.

Tools Used

Recommended Mitigation Steps

First Solution:

• The first deposit should be larger than a defined value.
• The amount that Bob should transfer directly to apply this attack is equal to:

Alice's deposit * Bob's first deposit / (Bob's first deposit + Bob's direct transfer) < 1  ==>  Bob's direct transfer = Bob's first deposit * (Alice's deposit - 1) + 1

• So, if the protocol defines that the first deposit to the protocol must be larger than 10, then in case Alice intends to deposit 100, Bob must directly transfer 10 * (100 - 1) + 1 = 991, this makes the attack for Bob much more difficult.
• There should be a delay between deposit and withdraw to make this attack more difficult for the attacker.

Second Solution:

To use MINIMUM_LIQUIDITY like unsiwap V2:

if (_totalSupply == 0) {
    liquidity = Math.sqrt(amount0.mul(amount1)).sub(MINIMUM_LIQUIDITY);
   _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
} else {
    liquidity = Math.min(amount0.mul(_totalSupply) / _reserve0, amount1.mul(_totalSupply) / _reserve1);
}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209

Vulnerability details

Impact

During upgrading the existing contract, first the new address is registered, then the old address is unregistered. If the name of the contract is going to be the same (it should be the same as it is hardcoded in the contract Ocyticus.sol), during unregistering the address of the contract will be set to address(0).

Proof of Concept

To restrict actions in important contracts, there is the function:

function pauseEverything() external onlyDefender {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		dao.pauseContract("TokenggAVAX");
		dao.pauseContract("MinipoolManager");
		dao.pauseContract("Staking");
		disableAllMultisigs();
	}

https://github.com/code-423n4/2022-12-gogopool/blob/1c30b320b7105e57c92232408bc795b6d2dfa208/contracts/contract/Ocyticus.sol#L37

It means that the important contracts will have the name TokenggAVAX, MinipoolManager, and Staking. In other words, if they are going to be deployed on different address, they should have still the same name, otherwise the contract Ocyticus.sol should be redeployed with the new names of these contracts, because the name of these contracts are hardcoded.

There is another function to upgrade the name and address of the contracts in the storage.

function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
		registerContract(newAddr, newName);
		unregisterContract(existingAddr);
	}

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209

The problem is that if for example the contract TokenggAVAX.sol is going to be upgraded, the function above will be called with the following parameters:

• newAddr = new address of the contracts TokenggAVAX.sol
• newName = TokenggAVAX (this should not be changed, otherwise the contract Ocyticus.sol should be redeployed as the name TokenggAVAX is hardcoded)
• existingAddr = old address of the contract TokenggAVAX.sol

In this function, first the new address will be registered:

function registerContract(address addr, string memory name) public onlyGuardian {
		setBool(keccak256(abi.encodePacked("contract.exists", addr)), true);
		setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);
		setString(keccak256(abi.encodePacked("contract.name", addr)), name);
	}

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L190

So, we will have:

• booleanStorage[H("contract.exists", new address)] = true
• addressStorage[H("contract.address", "TokenggAVAX")] = new address
• stringStorage[H("contract.name", new address)] = "TokenggAVAX"

Then, the old address is unregistered.

function unregisterContract(address addr) public onlyGuardian {
		string memory name = getContractName(addr);
		deleteBool(keccak256(abi.encodePacked("contract.exists", addr)));
		deleteAddress(keccak256(abi.encodePacked("contract.address", name)));
		deleteString(keccak256(abi.encodePacked("contract.name", addr)));
	}

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L198

So, we will have:

• booleanStorage[H("contract.exists", old address)] = false
• addressStorage[H("contract.address", "TokenggAVAX")] = address(0)
• stringStorage[H("contract.name", old address)] = ""

As you see, the problem is that the address of the contract TokenggAVAX.sol is set to address(0), while it was supposed to be equal to the new address.

Tools Used

Recommended Mitigation Steps

The following modifications should be applied (first should be unregistered, and then should be registered):

//before
function upgradeExistingContract(
    address newAddr,
    string memory newName,
    address existingAddr
) external onlyGuardian {
    registerContract(newAddr, newName);
    unregisterContract(existingAddr);
}

//after
function upgradeExistingContract(
    address newAddr,
    string memory newName,
    address existingAddr
) external onlyGuardian {
    unregisterContract(existingAddr);
    registerContract(newAddr, newName);
}