diff --git a/README.md b/README.md index 979639a..62f706a 100644 --- a/README.md +++ b/README.md @@ -6,14 +6,6 @@ This workshop will walk you through the basics of securing smart contracts by te In this workshop we will be protecting a governance heavy application from the consequences of malicious upgrades and or deployment scripts. -## Bug Types -- **Deployment** - Incorrect parameters, caught with a validation -- **Deployment** - Incorrect parameters, caught with an integration test ✓ -**** -- **Upgrade** - Storage offset changes, caught with an integration test, also caught with a validation -- **Upgrade** - Logic error, caught with an integration test -**** - ### Further Reading For governance safety assistance, refer to our [forge proposal simulator](https://github.com/solidity-labs-io/forge-proposal-simulator) tool. See the [security checklist](https://github.com/solidity-labs-io/code-review-checklist) and [security](https://medium.com/@elliotfriedman3/a-security-stack-4aedd8617e8b) [stack](https://medium.com/@elliotfriedman3/a-security-stack-part-2-aaacbbf77346) for a list of items to consider when building a smart contract system. @@ -31,11 +23,3 @@ Make sure the latest version of foundry is installed. If not, run: ```bash foundryup ``` - -Later exercises will use the certora prover. If you need to install, first check the system prerequisites from the Certora documentation. https://docs.certora.com/en/latest/docs/user-guide/install.html - -To install the prover run: - -```bash -pip3 install certora-cli -``` diff --git a/src/exercises/IVault.sol b/src/IVault.sol similarity index 100% rename from src/exercises/IVault.sol rename to src/IVault.sol diff --git a/src/exercises/03/Vault03.sol b/src/exercises/03/Vault03.sol index 43d4aa5..c09a193 100644 --- a/src/exercises/03/Vault03.sol +++ b/src/exercises/03/Vault03.sol @@ -10,7 +10,6 @@ import {VaultStorageOwnable} from "src/exercises/storage/VaultStorageOwnable.sol"; /// @notice Add maxsupply to the vault and update getNormalizedAmount logic -/// deploy Vault 03 to mainnet /// add integration tests contract Vault is VaultStorageOwnable { using SafeERC20 for IERC20; @@ -180,7 +179,7 @@ contract Vault is VaultStorageOwnable { uint8 decimals = IERC20Metadata(token).decimals(); normalizedAmount = amount; if (decimals < 18) { - normalizedAmount = amount ** (10 * (18 - decimals)); + normalizedAmount = amount * (10 ** (18 - decimals)); } } } diff --git a/src/exercises/04/SIP04.sol b/src/exercises/04/SIP04.sol index 0aaade3..eb5275f 100644 --- a/src/exercises/04/SIP04.sol +++ b/src/exercises/04/SIP04.sol @@ -70,6 +70,7 @@ contract SIP04 is GovernorBravoProposal { override buildModifier(addresses.getAddress("COMPOUND_TIMELOCK_BRAVO")) { + /// static calls - filtered out address vaultProxy = addresses.getAddress("VAULT_PROXY"); bytes32 adminSlot = vm.load(vaultProxy, ERC1967Utils.ADMIN_SLOT); @@ -115,6 +116,9 @@ contract SIP04 is GovernorBravoProposal { vm.load(vaultProxy, ERC1967Utils.ADMIN_SLOT); address proxyAdmin = address(uint160(uint256(adminSlot))); + /// check not paused + /// check logic contract address is v4 impl + assertEq( ProxyAdmin(proxyAdmin).owner(), addresses.getAddress("COMPOUND_TIMELOCK_BRAVO"), diff --git a/src/exercises/storage/VaultStoragePausable.sol b/src/exercises/storage/VaultStoragePausable.sol index afbb67f..d76be3a 100644 --- a/src/exercises/storage/VaultStoragePausable.sol +++ b/src/exercises/storage/VaultStoragePausable.sol @@ -12,7 +12,7 @@ import {Authorized} from "src/exercises/storage/Authorized.sol"; contract VaultStoragePausable is OwnableUpgradeable, Supply, + Authorized, Balances, - PausableUpgradeable, - Authorized + PausableUpgradeable {} diff --git a/src/proposals/sips/sips.json b/src/proposals/sips/sips.json deleted file mode 100644 index 402b137..0000000 --- a/src/proposals/sips/sips.json +++ /dev/null @@ -1,7 +0,0 @@ - -[ - { - "id": 0, - "path": "src/proposals/sips/SIP_00.sol" - } -] \ No newline at end of file diff --git a/test/TestVault01.t.sol b/test/TestVault01.t.sol new file mode 100644 index 0000000..360e938 --- /dev/null +++ b/test/TestVault01.t.sol @@ -0,0 +1,186 @@ +pragma solidity ^0.8.0; + +import {SafeERC20} from + "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import {console} from "@forge-std/console.sol"; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import {Test} from "@forge-std/Test.sol"; + +import {Vault} from "src/exercises/01/Vault01.sol"; +import {SIP01} from "src/exercises/01/SIP01.sol"; + +contract TestVault01 is Test, SIP01 { + using SafeERC20 for IERC20; + + Vault public vault; + + /// @notice user addresses + address public immutable userA = address(1111); + address public immutable userB = address(2222); + address public immutable userC = address(3333); + + /// @notice token addresses + address public usdc; + address public usdt; + + function setUp() public { + /// set the environment variables + vm.setEnv("DO_RUN", "false"); + vm.setEnv("DO_BUILD", "false"); + vm.setEnv("DO_DEPLOY", "true"); + vm.setEnv("DO_SIMULATE", "false"); + vm.setEnv("DO_PRINT", "false"); + vm.setEnv("DO_VALIDATE", "true"); + + /// setup the proposal + setupProposal(); + + /// run the proposal + deploy(); + + usdc = addresses.getAddress("USDC"); + usdt = addresses.getAddress("USDT"); + vault = Vault(addresses.getAddress("V1_VAULT")); + } + + function testVaultDepositUsdc() public { + uint256 usdcDepositAmount = 1_000e6; + + _vaultDeposit(usdc, address(this), usdcDepositAmount); + } + + function testMultipleUsersDepositUsdc() public { + uint256 usdcDepositAmount = 1_000e6; + + _vaultDeposit(usdc, userA, usdcDepositAmount); + _vaultDeposit(usdc, userB, usdcDepositAmount); + _vaultDeposit(usdc, userC, usdcDepositAmount); + } + + function testVaultWithdrawalUsdc() public { + uint256 usdcDepositAmount = 1_000e6; + + _vaultDeposit(usdc, address(this), usdcDepositAmount); + + vault.withdraw(usdc, usdcDepositAmount); + + assertEq( + vault.balanceOf(address(this)), + 0, + "vault usdc balance not 0" + ); + assertEq( + vault.totalSupplied(), 0, "vault total supplied not 0" + ); + assertEq( + IERC20(usdc).balanceOf(address(this)), + usdcDepositAmount, + "user's usdc balance not increased" + ); + } + + function testVaultDepositUsdt() public { + uint256 usdtDepositAmount = 1_000e8; + + _vaultDeposit(usdt, address(this), usdtDepositAmount); + } + + function testVaultWithdrawalUsdt() public { + uint256 usdtDepositAmount = 1_000e8; + + _vaultDeposit(usdt, address(this), usdtDepositAmount); + vault.withdraw(usdt, usdtDepositAmount); + + assertEq( + vault.balanceOf(address(this)), + 0, + "vault usdt balance not 0" + ); + assertEq( + vault.totalSupplied(), 0, "vault total supplied not 0" + ); + assertEq( + IERC20(usdt).balanceOf(address(this)), + usdtDepositAmount, + "user's usdt balance not increased" + ); + } + + function testSwapTwoUsers() public { + uint256 usdcDepositAmount = 1_000e6; + uint256 usdtDepositAmount = 1_000e8; + + _vaultDeposit(usdc, userA, usdcDepositAmount); + _vaultDeposit(usdt, userB, usdtDepositAmount); + + vm.prank(userA); + vault.withdraw(usdt, usdcDepositAmount); + assertEq( + IERC20(usdt).balanceOf(userA), + usdcDepositAmount, + "userA usdt balance not increased" + ); + + vm.prank(userB); + vault.withdraw(usdc, usdcDepositAmount); + assertEq( + IERC20(usdc).balanceOf(userB), + usdcDepositAmount, + "userB usdc balance not increased" + ); + assertEq( + IERC20(usdt).balanceOf(userA), + usdcDepositAmount, + "userB usdt balance remains unchanged" + ); + } + + function _vaultDeposit( + address token, + address sender, + uint256 amount + ) private { + uint256 startingTotalSupplied = vault.totalSupplied(); + uint256 startingTotalBalance = + IERC20(token).balanceOf(address(vault)); + uint256 startingUserBalance = vault.balanceOf(sender); + + deal(token, sender, amount); + + vm.startPrank(sender); + IERC20(token).safeIncreaseAllowance( + addresses.getAddress("V1_VAULT"), amount + ); + + /// this executes 3 state transitions: + /// 1. deposit dai into the vault + /// 2. increase the user's balance in the vault + /// 3. increase the total supplied amount in the vault + vault.deposit(token, amount); + vm.stopPrank(); + + uint256 normalizedAmount = + vault.getNormalizedAmount(token, amount); + + assertEq( + vault.balanceOf(sender), + startingUserBalance + normalizedAmount, + "user vault balance not increased" + ); + assertEq( + vault.totalSupplied(), + startingTotalSupplied + normalizedAmount, + "vault total supplied not increased by deposited amount" + ); + assertEq( + IERC20(token).balanceOf(address(vault)), + startingTotalBalance + amount, + "token balance not increased" + ); + } +} + +interface USDT { + function approve(address, uint256) external; + function transferFrom(address, address, uint256) external; +} diff --git a/test/TestVault02.t.sol b/test/TestVault02.t.sol index 6ca5bf3..3e75d90 100644 --- a/test/TestVault02.t.sol +++ b/test/TestVault02.t.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.0; import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import {console} from "@forge-std/console.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {Test} from "@forge-std/Test.sol"; @@ -79,10 +78,6 @@ contract TestVault02 is Test, SIP02 { addresses.getAddress("V2_VAULT"), usdtDepositAmount ); - /// this executes 3 state transitions: - /// 1. deposit dai into the vault - /// 2. increase the user's balance in the vault - /// 3. increase the total supplied amount in the vault vault.deposit(usdt, usdtDepositAmount); assertEq( @@ -141,6 +136,11 @@ contract TestVault02 is Test, SIP02 { vm.prank(userB); vault.withdraw(usdc, usdcDepositAmount); + assertEq( + IERC20(usdc).balanceOf(userB), + usdcDepositAmount, + "userB usdc balance not increased" + ); assertEq( IERC20(usdt).balanceOf(userA), usdcDepositAmount, diff --git a/test/TestVault03.t.sol b/test/TestVault03.t.sol new file mode 100644 index 0000000..7a60912 --- /dev/null +++ b/test/TestVault03.t.sol @@ -0,0 +1,211 @@ +pragma solidity ^0.8.0; + +import {SafeERC20} from + "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import {Test, console} from "@forge-std/Test.sol"; + +import {SIP03} from "src/exercises/03/SIP03.sol"; +import {Vault} from "src/exercises/03/Vault03.sol"; + +contract TestVault03 is Test, SIP03 { + using SafeERC20 for IERC20; + + Vault public vault; + + /// @notice user addresses + address public immutable userA = address(1111); + address public immutable userB = address(2222); + address public immutable userC = address(3333); + + /// @notice token addresses + address public dai; + address public usdc; + address public usdt; + + function _loadUsers() private { + address[] memory users = new address[](3); + users[0] = userA; + users[1] = userB; + users[2] = userC; + + for (uint256 i = 0; i < users.length; i++) { + uint256 daiDepositAmount = 1_000e18; + uint256 usdtDepositAmount = 1_000e8; + uint256 usdcDepositAmount = 1_000e6; + + _vaultDeposit(dai, users[i], daiDepositAmount); + _vaultDeposit(usdc, users[i], usdcDepositAmount); + _vaultDeposit(usdt, users[i], usdtDepositAmount); + } + } + + function setUp() public { + /// set the environment variables + vm.setEnv("DO_RUN", "false"); + vm.setEnv("DO_BUILD", "false"); + vm.setEnv("DO_DEPLOY", "true"); + vm.setEnv("DO_SIMULATE", "false"); + vm.setEnv("DO_PRINT", "false"); + vm.setEnv("DO_VALIDATE", "false"); + + setupProposal(); + + deploy(); + + dai = addresses.getAddress("DAI"); + usdc = addresses.getAddress("USDC"); + usdt = addresses.getAddress("USDT"); + vault = Vault(addresses.getAddress("VAULT_PROXY")); + + vm.prank(vault.owner()); + vault.setMaxSupply(100_000_000e18); + + /// load data into newly deployed contract + _loadUsers(); + } + + function testSetup() public view { + validate(); + assertEq( + vault.maxSupply(), 100_000_000e18, "max supply not set" + ); + assertEq( + vault.totalSupplied(), + ( + vault.getNormalizedAmount(dai, 1_000e18) + + vault.getNormalizedAmount(usdc, 1_000e6) + + vault.getNormalizedAmount(usdt, 1_000e8) + ) * 3, + "total supplied not set" + ); + } + + function testVaultDepositDai() public { + uint256 daiDepositAmount = 1_000e18; + + _vaultDeposit(dai, address(this), daiDepositAmount); + } + + function testVaultWithdrawalDai() public { + uint256 daiDepositAmount = 1_000e18; + + _vaultDeposit(dai, address(this), daiDepositAmount); + uint256 startingVaultBalance = vault.balanceOf(address(this)); + uint256 startingTotalSupplied = vault.totalSupplied(); + + vault.withdraw(dai, daiDepositAmount); + + assertEq( + vault.balanceOf(address(this)), + startingVaultBalance - daiDepositAmount, + "vault dai balance not 0" + ); + assertEq( + vault.totalSupplied(), + startingTotalSupplied - daiDepositAmount, + "vault total supplied not 0" + ); + assertEq( + IERC20(dai).balanceOf(address(this)), + daiDepositAmount, + "user's dai balance not increased" + ); + } + + function testVaultWithdrawUSDC() public { + uint256 usdcDepositAmount = 1_000e6; + + _vaultDeposit(usdc, address(this), usdcDepositAmount); + uint256 startingVaultBalance = vault.balanceOf(address(this)); + uint256 startingTotalSupplied = vault.totalSupplied(); + + vault.withdraw(usdc, usdcDepositAmount); + + assertEq( + vault.balanceOf(address(this)), + startingVaultBalance + - vault.getNormalizedAmount(usdc, usdcDepositAmount), + "vault usdc balance not 0" + ); + assertEq( + vault.totalSupplied(), + startingTotalSupplied + - vault.getNormalizedAmount(usdc, usdcDepositAmount), + "vault total supplied not 0" + ); + assertEq( + IERC20(usdc).balanceOf(address(this)), + usdcDepositAmount, + "user's usdc balance not increased" + ); + } + + function testVaultWithdrawUSDT() public { + uint256 usdtDepositAmount = 1_000e8; + + _vaultDeposit(usdt, address(this), usdtDepositAmount); + uint256 startingVaultBalance = vault.balanceOf(address(this)); + uint256 startingTotalSupplied = vault.totalSupplied(); + + vault.withdraw(usdt, usdtDepositAmount); + + assertEq( + vault.balanceOf(address(this)), + startingVaultBalance + - vault.getNormalizedAmount(usdt, usdtDepositAmount), + "vault usdt balance not 0" + ); + assertEq( + vault.totalSupplied(), + startingTotalSupplied + - vault.getNormalizedAmount(usdt, usdtDepositAmount), + "vault total supplied not 0" + ); + assertEq( + IERC20(usdt).balanceOf(address(this)), + usdtDepositAmount, + "user's usdt balance not increased" + ); + } + + function _vaultDeposit( + address token, + address sender, + uint256 amount + ) private { + uint256 startingTotalSupplied = vault.totalSupplied(); + uint256 startingTotalBalance = + IERC20(token).balanceOf(address(vault)); + uint256 startingUserBalance = vault.balanceOf(sender); + + deal(token, sender, amount); + + vm.startPrank(sender); + IERC20(token).safeIncreaseAllowance( + addresses.getAddress("VAULT_PROXY"), amount + ); + + vault.deposit(token, amount); + vm.stopPrank(); + + uint256 normalizedAmount = + vault.getNormalizedAmount(token, amount); + + assertEq( + vault.balanceOf(sender), + startingUserBalance + normalizedAmount, + "user vault balance not increased" + ); + assertEq( + vault.totalSupplied(), + startingTotalSupplied + normalizedAmount, + "vault total supplied not increased by deposited amount" + ); + assertEq( + IERC20(token).balanceOf(address(vault)), + startingTotalBalance + amount, + "token balance not increased" + ); + } +} diff --git a/test/TestVault04.t.sol b/test/TestVault04.t.sol index 6ecc0a6..5936333 100644 --- a/test/TestVault04.t.sol +++ b/test/TestVault04.t.sol @@ -4,6 +4,10 @@ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {Test, console} from "@forge-std/Test.sol"; +import {ERC1967Utils} from + "@openzeppelin/contracts/proxy/ERC1967/ERC1967Utils.sol"; +import {ProxyAdmin} from + "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; import {SIP03} from "src/exercises/03/SIP03.sol"; import {SIP04} from "src/exercises/04/SIP04.sol"; @@ -38,10 +42,20 @@ contract TestVault04 is Test, SIP04 { sip03.setupProposal(); sip03.deploy(); + /// set the addresses contrac to the SIP03 addresses for integration testing + setAddresses(sip03.addresses()); + dai = addresses.getAddress("DAI"); + usdc = addresses.getAddress("USDC"); + usdt = addresses.getAddress("USDT"); + vault = Vault(addresses.getAddress("VAULT_PROXY")); + + vm.prank(vault.owner()); + vault.setMaxSupply(100_000_000e18); + /// setup the proposal setupProposal(); - /// copy SIP03 addresses into this contract for integration testing + /// overwrite the newly created proposal Addresses contract setAddresses(sip03.addresses()); /// deploy contracts from MIP-04 @@ -50,90 +64,21 @@ contract TestVault04 is Test, SIP04 { /// build and run proposal build(); simulate(); - - dai = addresses.getAddress("DAI"); - usdc = addresses.getAddress("USDC"); - usdt = addresses.getAddress("USDT"); - vault = Vault(addresses.getAddress("VAULT_PROXY")); - } - - function testVaultDepositDai() public { - uint256 daiDepositAmount = 1_000e18; - - _vaultDeposit(dai, address(this), daiDepositAmount); } - function testVaultWithdrawalDai() public { - uint256 daiDepositAmount = 1_000e18; - - _vaultDeposit(dai, address(this), daiDepositAmount); - - vault.withdraw(dai, daiDepositAmount); - - assertEq( - vault.balanceOf(address(this)), - 0, - "vault dai balance not 0" - ); - assertEq( - vault.totalSupplied(), 0, "vault total supplied not 0" - ); + function testValidate() public view { assertEq( - IERC20(dai).balanceOf(address(this)), - daiDepositAmount, - "user's dai balance not increased" + vault.maxSupply(), 1_000_000e18, "max supply not set" ); - } - - function testWithdrawAlreadyDepositedUSDC() public { - uint256 usdcDepositAmount = 1_000e6; - _vaultDeposit(usdc, address(this), usdcDepositAmount); - - vault.withdraw(usdc, usdcDepositAmount); - } + bytes32 adminSlot = + vm.load(address(vault), ERC1967Utils.ADMIN_SLOT); + address proxyAdmin = address(uint160(uint256(adminSlot))); - function _vaultDeposit( - address token, - address sender, - uint256 amount - ) private { - uint256 startingTotalSupplied = vault.totalSupplied(); - uint256 startingTotalBalance = - IERC20(token).balanceOf(address(vault)); - uint256 startingUserBalance = vault.balanceOf(sender); - - deal(token, sender, amount); - - vm.startPrank(sender); - IERC20(token).safeIncreaseAllowance( - addresses.getAddress("VAULT_PROXY"), amount - ); - - /// this executes 3 state transitions: - /// 1. deposit dai into the vault - /// 2. increase the user's balance in the vault - /// 3. increase the total supplied amount in the vault - vault.deposit(token, amount); - vm.stopPrank(); - - uint256 normalizedAmount = - vault.getNormalizedAmount(token, amount); - - assertEq( - vault.balanceOf(sender), - startingUserBalance + normalizedAmount, - "user vault balance not increased" - ); - assertEq( - vault.totalSupplied(), - startingTotalSupplied + normalizedAmount, - "vault total supplied not increased by deposited amount" - ); assertEq( - IERC20(token).balanceOf(address(vault)), - startingTotalBalance + amount, - "token balance not increased" + ProxyAdmin(proxyAdmin).owner(), + addresses.getAddress("COMPOUND_TIMELOCK_BRAVO"), + "owner not set" ); } }