GoGoPool contest - peanuts's results

Lines of code

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#L24

Vulnerability details

The calculation of exchange rate for shares in TokenggAVAX Vault is done by dividing the total supply of shares by the totalAssets of the vault. The first depositor can mint a very small number of shares, then donate to the vault to manipulate the share price. When subsequent depositors deposit, they will lose value due to precision loss. This is a common attack vector for almost all shares based liquidity pool contracts using ERC4626.

Impact

First depositor can manipulate shares from later users; later users will not get equivalent shares when converting their underlying asset.

Proof of Concept

1. Malicious user Alice can deposit() with 1 wei of asset token to get 1 wei of shares.

2. Next, Alice sends 10000e18 -1 of asset tokens and inflate the price per share from 1 to 1e22.

3. Subsequent depositor who deposits shares, eg 19999e18 of assets, will only receive 1 wei of shares token.

4. Victim will lose 9999e18 if they redeem() right after deposit() due to precision loss.

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

Reference:

https://github.com/sherlock-audit/2022-08-sentiment-judging#issue-h-1-a-malicious-early-userattacker-can-manipulate-the-ltokens-pricepershare-to-take-an-unfair-share-of-future-users-deposits

Tools Used

Manual Review

Recommended Mitigation Steps

Consider requiring a minimum amount of share tokens to be minted for the first minter or follow Uniswap V2 which mints 10,000 share first to balance liquidity.

https://github.com/Uniswap/v2-core/blob/ee547b17853e71ed4e0101ccfd52e70d5acded58/contracts/UniswapV2Pair.sol#L119-L124

Lines of code

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

Vulnerability details

Impact

Guardian may accidentally delete a contract. Might waste gas and also cause confusion to the users and the guardian himself.

Proof of Concept

When the protocol decides to upgrade its contract, it calls upgradeExistingContract in ProtocolDAO.sol. The upgradeExistingContract calls registerContract and registers a new address and name, then unregisterContract to delete the old contract address and name.

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

If the guardian accidentally uses the same new address for both new and existing addresses, then there will be nothing to upgrade.

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

Recommended Mitigation Steps

Make sure newAddr is not equal to existing Addr and newAddr is not the same as current Addr.