C4-300: Add storage gap to ERC20, ERC4626, BaseUpgradeable (#47)

* Add storage gap and tests

contracts/contract/BaseUpgradeable.sol

@@ -10,4 +10,8 @@ contract BaseUpgradeable is Initializable, BaseAbstract {
 	function __BaseUpgradeable_init(Storage gogoStorageAddress) internal onlyInitializing {
 		gogoStorage = Storage(gogoStorageAddress);
 	}
+
+	/// @dev This empty reserved space is put in place to allow future versions to add new
+	/// variables without shifting down storage in the inheritance chain.
+	uint256[50] private __gap;
 }

contracts/contract/tokens/upgradeable/ERC20Upgradeable.sol

@@ -205,4 +205,8 @@ abstract contract ERC20Upgradeable is Initializable {
 
 		emit Transfer(from, address(0), amount);
 	}
+
+	/// @dev This empty reserved space is put in place to allow future versions to add new
+	/// variables without shifting down storage in the inheritance chain.
+	uint256[50] private __gap;
 }

contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol

@@ -176,4 +176,8 @@ abstract contract ERC4626Upgradeable is Initializable, ERC20Upgradeable {
 	function beforeWithdraw(uint256 assets, uint256 shares) internal virtual {}
 
 	function afterDeposit(uint256 assets, uint256 shares) internal virtual {}
+
+	/// @dev This empty reserved space is put in place to allow future versions to add new
+	/// variables without shifting down storage in the inheritance chain.
+	uint256[50] private __gap;
 }

test/unit/TokenUpgradeTests.t.sol

@@ -5,6 +5,8 @@ pragma solidity 0.8.17;
 import "./utils/BaseTest.sol";
 
 import {MockTokenggAVAXV2} from "./utils/MockTokenggAVAXV2.sol";
+import {MockTokenggAVAXV2Dangerous} from "./utils/MockTokenggAVAXV2Dangerous.sol";
+import {MockTokenggAVAXV2Safe} from "./utils/MockTokenggAVAXV2Safe.sol";
 
 contract TokenUpgradeTests is BaseTest {
 	function setUp() public override {
@@ -33,4 +35,59 @@ contract TokenUpgradeTests is BaseTest {
 		assertEq(address(proxy), oldAddress);
 		assertEq(proxy.name(), oldName);
 	}
+
+	function testStorageGapDangerouslySet() public {
+		// initialize token
+		TokenggAVAX impl = new TokenggAVAX();
+		TokenggAVAX proxy = TokenggAVAX(deployProxy(address(impl), guardian));
+
+		proxy.initialize(store, wavax);
+
+		proxy.syncRewards();
+		vm.warp(ggAVAX.rewardsCycleEnd());
+
+		// add some rewards to make sure error error occurs
+		address alice = getActorWithTokens("alice", 1000 ether, 0 ether);
+		vm.prank(alice);
+		wavax.transfer(address(proxy), 1000 ether);
+		proxy.syncRewards();
+
+		uint256 oldLastSync = proxy.lastSync();
+		bytes32 oldDomainSeparator = proxy.DOMAIN_SEPARATOR();
+
+		// upgrade implementation
+		MockTokenggAVAXV2Dangerous impl2 = new MockTokenggAVAXV2Dangerous();
+		vm.prank(guardian);
+		proxy.upgradeTo(address(impl2));
+		proxy.initialize(store, wavax);
+
+		// now lastSync is reading four bytes of lastRewardsAmt
+		assertFalse(proxy.lastSync() == oldLastSync);
+
+		// domain separator also does not change but should during regular upgrade
+		assertEq(proxy.DOMAIN_SEPARATOR(), oldDomainSeparator);
+	}
+
+	function testStorageGapSafe() public {
+		// initialize token
+		TokenggAVAX impl = new TokenggAVAX();
+		TokenggAVAX proxy = TokenggAVAX(deployProxy(address(impl), guardian));
+
+		proxy.initialize(store, wavax);
+
+		proxy.syncRewards();
+		uint256 oldLastSync = proxy.lastSync();
+		bytes32 oldDomainSeparator = proxy.DOMAIN_SEPARATOR();
+
+		// upgrade implementation
+		MockTokenggAVAXV2Safe impl2 = new MockTokenggAVAXV2Safe();
+		vm.prank(guardian);
+		proxy.upgradeTo(address(impl2));
+		proxy.initialize(store, wavax);
+
+		// verify that lastSync is not overwritten during upgrade
+		assertEq(proxy.lastSync(), oldLastSync);
+		// verify domain separator changes
+		assertFalse(proxy.DOMAIN_SEPARATOR() == oldDomainSeparator);
+	}
 }

test/unit/utils/ERC20UpgradeableDangerous.sol

@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: AGPL-3.0-only
+pragma solidity >=0.8.0;
+
+import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+
+/// @notice Modern and gas efficient ERC20 + EIP-2612 implementation.
+/// @author Solmate (https://github.com/Rari-Capital/solmate/blob/main/src/tokens/ERC20.sol)
+/// @author Modified from Uniswap (https://github.com/Uniswap/uniswap-v2-core/blob/master/contracts/UniswapV2ERC20.sol)
+/// @dev Do not manually set balances without updating totalSupply, as the sum of all user balances must not exceed it.
+abstract contract ERC20UpgradeableDangerous is Initializable {
+	/*//////////////////////////////////////////////////////////////
+                                 EVENTS
+    //////////////////////////////////////////////////////////////*/
+
+	event Transfer(address indexed from, address indexed to, uint256 amount);
+
+	event Approval(address indexed owner, address indexed spender, uint256 amount);
+
+	/*//////////////////////////////////////////////////////////////
+                            METADATA STORAGE
+    //////////////////////////////////////////////////////////////*/
+
+	string public name;
+
+	string public symbol;
+
+	uint8 public decimals;
+
+	/*//////////////////////////////////////////////////////////////
+                              ERC20 STORAGE
+    //////////////////////////////////////////////////////////////*/
+
+	uint256 public totalSupply;
+
+	mapping(address => uint256) public balanceOf;
+
+	mapping(address => mapping(address => uint256)) public allowance;
+
+	/*//////////////////////////////////////////////////////////////
+                            EIP-2612 STORAGE
+    //////////////////////////////////////////////////////////////*/
+
+	uint256 internal INITIAL_CHAIN_ID;
+
+	bytes32 internal INITIAL_DOMAIN_SEPARATOR;
+
+	mapping(address => uint256) public nonces;
+
+	// New storage variable that we do NOT account for in the gap
+	uint256 public dangerousVariable;
+
+	/*//////////////////////////////////////////////////////////////
+                               CONSTRUCTOR
+    //////////////////////////////////////////////////////////////*/
+
+	function __ERC20Upgradeable_init(
+		string memory _name,
+		string memory _symbol,
+		uint8 _decimals
+	) internal onlyInitializing {
+		name = _name;
+		symbol = _symbol;
+		decimals = _decimals;
+
+		INITIAL_CHAIN_ID = block.chainid;
+		INITIAL_DOMAIN_SEPARATOR = computeDomainSeparator();
+	}
+
+	/*//////////////////////////////////////////////////////////////
+                               ERC20 LOGIC
+    //////////////////////////////////////////////////////////////*/
+
+	function approve(address spender, uint256 amount) public virtual returns (bool) {
+		allowance[msg.sender][spender] = amount;
+
+		emit Approval(msg.sender, spender, amount);
+
+		return true;
+	}
+
+	function transfer(address to, uint256 amount) public virtual returns (bool) {
+		balanceOf[msg.sender] -= amount;
+
+		// Cannot overflow because the sum of all user
+		// balances can't exceed the max uint256 value.
+		unchecked {
+			balanceOf[to] += amount;
+		}
+
+		emit Transfer(msg.sender, to, amount);
+
+		return true;
+	}
+
+	function transferFrom(
+		address from,
+		address to,
+		uint256 amount
+	) public virtual returns (bool) {
+		uint256 allowed = allowance[from][msg.sender]; // Saves gas for limited approvals.
+
+		if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amount;
+
+		balanceOf[from] -= amount;
+
+		// Cannot overflow because the sum of all user
+		// balances can't exceed the max uint256 value.
+		unchecked {
+			balanceOf[to] += amount;
+		}
+
+		emit Transfer(from, to, amount);
+
+		return true;
+	}
+
+	/*//////////////////////////////////////////////////////////////
+                             EIP-2612 LOGIC
+    //////////////////////////////////////////////////////////////*/
+
+	function permit(
+		address owner,
+		address spender,
+		uint256 value,
+		uint256 deadline,
+		uint8 v,
+		bytes32 r,
+		bytes32 s
+	) public virtual {
+		require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED");
+
+		// Unchecked because the only math done is incrementing
+		// the owner's nonce which cannot realistically overflow.
+		unchecked {
+			address recoveredAddress = ecrecover(
+				keccak256(
+					abi.encodePacked(
+						"\x19\x01",
+						DOMAIN_SEPARATOR(),
+						keccak256(
+							abi.encode(
+								keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"),
+								owner,
+								spender,
+								value,
+								nonces[owner]++,
+								deadline
+							)
+						)
+					)
+				),
+				v,
+				r,
+				s
+			);
+
+			require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");
+
+			allowance[recoveredAddress][spender] = value;
+		}
+
+		emit Approval(owner, spender, value);
+	}
+
+	function DOMAIN_SEPARATOR() public view virtual returns (bytes32) {
+		return block.chainid == INITIAL_CHAIN_ID ? INITIAL_DOMAIN_SEPARATOR : computeDomainSeparator();
+	}
+
+	function computeDomainSeparator() internal view virtual returns (bytes32) {
+		return
+			keccak256(
+				abi.encode(
+					keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"),
+					keccak256(bytes(name)),
+					versionHash(),
+					block.chainid,
+					address(this)
+				)
+			);
+	}
+
+	function versionHash() internal view virtual returns (bytes32);
+
+	/*//////////////////////////////////////////////////////////////
+                        INTERNAL MINT/BURN LOGIC
+    //////////////////////////////////////////////////////////////*/
+
+	function _mint(address to, uint256 amount) internal virtual {
+		totalSupply += amount;
+
+		// Cannot overflow because the sum of all user
+		// balances can't exceed the max uint256 value.
+		unchecked {
+			balanceOf[to] += amount;
+		}
+
+		emit Transfer(address(0), to, amount);
+	}
+
+	function _burn(address from, uint256 amount) internal virtual {
+		balanceOf[from] -= amount;
+
+		// Cannot underflow because a user's balance
+		// will never be larger than the total supply.
+		unchecked {
+			totalSupply -= amount;
+		}
+
+		emit Transfer(from, address(0), amount);
+	}
+
+	/// @dev This empty reserved space is put in place to allow future versions to add new
+	/// variables without shifting down storage in the inheritance chain.
+	uint256[50] private __gap;
+}