From 7c777f926b9c63b75ad4728e4e5da7cb9441c3ca Mon Sep 17 00:00:00 2001 From: miguelmtz <36620902+miguelmtzinf@users.noreply.github.com> Date: Tue, 11 Jun 2024 10:28:29 +0200 Subject: [PATCH] fix: Apply fixes based on code review (#11) * fix: Use modern version of Initializable * fix: Update diffs * fix: Add event emission on bridge limit admin update * fix: Rebuild GHO diffs * fix: Fix certora and test config files * fix: Remove unneeded revision getter --- .gitmodules | 3 + certora/confs/ccip.conf | 3 + contracts/foundry-lib/gho-core | 1 + contracts/remappings.txt | 2 +- .../GHO/UpgradeableBurnMintTokenPool.sol | 18 +--- .../GHO/UpgradeableLockReleaseTokenPool.sol | 27 ++---- .../ccip/pools/GHO/VersionedInitializable.sol | 77 ---------------- ...radeableBurnMintTokenPoolAbstract_diff.md} | 15 ++-- ...d => UpgradeableBurnMintTokenPool_diff.md} | 49 +++++------ ...> UpgradeableLockReleaseTokenPool_diff.md} | 87 ++++++++----------- ...l_diff.md => UpgradeableTokenPool_diff.md} | 36 +++++--- .../v0.8/ccip/test/mocks/MockUpgradeable.sol | 19 +--- .../ccip/test/pools/GHO/GhoBaseTest.t.sol | 13 +-- .../test/pools/GHO/GhoTokenPoolEthereum.t.sol | 19 +++- .../test/pools/GHO/GhoTokenPoolRemote.t.sol | 8 +- ...okenPoolEthereumBridgeLimitInvariant.t.sol | 4 + .../GHO/invariant/GhoTokenPoolHandler.t.sol | 19 ++-- 17 files changed, 155 insertions(+), 245 deletions(-) create mode 160000 contracts/foundry-lib/gho-core delete mode 100644 contracts/src/v0.8/ccip/pools/GHO/VersionedInitializable.sol rename contracts/src/v0.8/ccip/pools/GHO/diffs/{BurnMintTokenPoolAbstract_diff.md => UpgradeableBurnMintTokenPoolAbstract_diff.md} (68%) rename contracts/src/v0.8/ccip/pools/GHO/diffs/{BurnMintTokenPool_diff.md => UpgradeableBurnMintTokenPool_diff.md} (76%) rename contracts/src/v0.8/ccip/pools/GHO/diffs/{LockReleaseTokenPool_diff.md => UpgradeableLockReleaseTokenPool_diff.md} (80%) rename contracts/src/v0.8/ccip/pools/GHO/diffs/{TokenPool_diff.md => UpgradeableTokenPool_diff.md} (61%) diff --git a/.gitmodules b/.gitmodules index 21fec76324..a8b5a9aeaf 100644 --- a/.gitmodules +++ b/.gitmodules @@ -4,3 +4,6 @@ [submodule "contracts/foundry-lib/solidity-utils"] path = contracts/foundry-lib/solidity-utils url = https://github.com/bgd-labs/solidity-utils +[submodule "contracts/foundry-lib/gho-core"] + path = contracts/foundry-lib/gho-core + url = https://github.com/aave/gho-core diff --git a/certora/confs/ccip.conf b/certora/confs/ccip.conf index 003f089502..0bd201b8d4 100644 --- a/certora/confs/ccip.conf +++ b/certora/confs/ccip.conf @@ -3,6 +3,9 @@ "contracts/src/v0.8/ccip/pools/GHO/UpgradeableLockReleaseTokenPool.sol", "certora/harness/SimpleERC20.sol" ], + "packages": [ + "solidity-utils/=contracts/foundry-lib/solidity-utils/src/" + ], "link": [ "UpgradeableLockReleaseTokenPool:i_token=SimpleERC20" ], diff --git a/contracts/foundry-lib/gho-core b/contracts/foundry-lib/gho-core new file mode 160000 index 0000000000..a8d05e6e72 --- /dev/null +++ b/contracts/foundry-lib/gho-core @@ -0,0 +1 @@ +Subproject commit a8d05e6e72409aa5ea6fd84d8a3c41e13887654d diff --git a/contracts/remappings.txt b/contracts/remappings.txt index 8fbfa33413..665a6d78a0 100644 --- a/contracts/remappings.txt +++ b/contracts/remappings.txt @@ -6,5 +6,5 @@ forge-std/=foundry-lib/forge-std/src/ hardhat/=node_modules/hardhat/ @eth-optimism/=node_modules/@eth-optimism/ @scroll-tech/=node_modules/@scroll-tech/ -@aave/gho-core/=node_modules/@aave/gho/src/contracts/ +@aave/gho-core/=foundry-lib/gho-core/src/contracts/ solidity-utils/=foundry-lib/solidity-utils/src/ diff --git a/contracts/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPool.sol b/contracts/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPool.sol index cc0f24af39..58be87812f 100644 --- a/contracts/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPool.sol +++ b/contracts/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPool.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity ^0.8.0; +import {Initializable} from "solidity-utils/contracts/transparent-proxy/Initializable.sol"; + import {ITypeAndVersion} from "../../../shared/interfaces/ITypeAndVersion.sol"; import {IBurnMintERC20} from "../../../shared/token/ERC20/IBurnMintERC20.sol"; @@ -8,15 +10,14 @@ import {UpgradeableTokenPool} from "./UpgradeableTokenPool.sol"; import {UpgradeableBurnMintTokenPoolAbstract} from "./UpgradeableBurnMintTokenPoolAbstract.sol"; import {IRouter} from "../../interfaces/IRouter.sol"; -import {VersionedInitializable} from "./VersionedInitializable.sol"; /// @title UpgradeableBurnMintTokenPool /// @author Aave Labs /// @notice Upgradeable version of Chainlink's CCIP BurnMintTokenPool /// @dev Contract adaptations: -/// - Implementation of VersionedInitializable to allow upgrades +/// - Implementation of Initializable to allow upgrades /// - Move of allowlist and router definition to initialization stage -contract UpgradeableBurnMintTokenPool is VersionedInitializable, UpgradeableBurnMintTokenPoolAbstract, ITypeAndVersion { +contract UpgradeableBurnMintTokenPool is Initializable, UpgradeableBurnMintTokenPoolAbstract, ITypeAndVersion { string public constant override typeAndVersion = "BurnMintTokenPool 1.4.0"; /// @dev Constructor @@ -52,15 +53,4 @@ contract UpgradeableBurnMintTokenPool is VersionedInitializable, UpgradeableBurn function _burn(uint256 amount) internal virtual override { IBurnMintERC20(address(i_token)).burn(amount); } - - /// @notice Returns the revision number - /// @return The revision number - function REVISION() public pure virtual returns (uint256) { - return 1; - } - - /// @inheritdoc VersionedInitializable - function getRevision() internal pure virtual override returns (uint256) { - return REVISION(); - } } diff --git a/contracts/src/v0.8/ccip/pools/GHO/UpgradeableLockReleaseTokenPool.sol b/contracts/src/v0.8/ccip/pools/GHO/UpgradeableLockReleaseTokenPool.sol index 0fac98c708..9a30b1e977 100644 --- a/contracts/src/v0.8/ccip/pools/GHO/UpgradeableLockReleaseTokenPool.sol +++ b/contracts/src/v0.8/ccip/pools/GHO/UpgradeableLockReleaseTokenPool.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity ^0.8.0; +import {Initializable} from "solidity-utils/contracts/transparent-proxy/Initializable.sol"; + import {ITypeAndVersion} from "../../../shared/interfaces/ITypeAndVersion.sol"; import {ILiquidityContainer} from "../../../rebalancer/interfaces/ILiquidityContainer.sol"; @@ -11,21 +13,15 @@ import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/tok import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol"; import {IRouter} from "../../interfaces/IRouter.sol"; -import {VersionedInitializable} from "./VersionedInitializable.sol"; /// @title UpgradeableLockReleaseTokenPool /// @author Aave Labs /// @notice Upgradeable version of Chainlink's CCIP LockReleaseTokenPool /// @dev Contract adaptations: -/// - Implementation of VersionedInitializable to allow upgrades +/// - Implementation of Initializable to allow upgrades /// - Move of allowlist and router definition to initialization stage /// - Addition of a bridge limit to regulate the maximum amount of tokens that can be transferred out (burned/locked) -contract UpgradeableLockReleaseTokenPool is - VersionedInitializable, - UpgradeableTokenPool, - ILiquidityContainer, - ITypeAndVersion -{ +contract UpgradeableLockReleaseTokenPool is Initializable, UpgradeableTokenPool, ILiquidityContainer, ITypeAndVersion { using SafeERC20 for IERC20; error InsufficientLiquidity(); @@ -34,7 +30,9 @@ contract UpgradeableLockReleaseTokenPool is error BridgeLimitExceeded(uint256 bridgeLimit); error NotEnoughBridgedAmount(); + event BridgeLimitUpdated(uint256 oldBridgeLimit, uint256 newBridgeLimit); + event BridgeLimitAdminUpdated(address indexed oldAdmin, address indexed newAdmin); string public constant override typeAndVersion = "LockReleaseTokenPool 1.4.0"; @@ -197,7 +195,9 @@ contract UpgradeableLockReleaseTokenPool is /// @dev Only callable by the owner. /// @param bridgeLimitAdmin The new bridge limit admin address. function setBridgeLimitAdmin(address bridgeLimitAdmin) external onlyOwner { + address oldAdmin = s_bridgeLimitAdmin; s_bridgeLimitAdmin = bridgeLimitAdmin; + emit BridgeLimitAdminUpdated(oldAdmin, bridgeLimitAdmin); } /// @notice Gets the bridge limit @@ -263,15 +263,4 @@ contract UpgradeableLockReleaseTokenPool is _setRateLimitConfig(remoteChainSelector, outboundConfig, inboundConfig); } - - /// @notice Returns the revision number - /// @return The revision number - function REVISION() public pure virtual returns (uint256) { - return 1; - } - - /// @inheritdoc VersionedInitializable - function getRevision() internal pure virtual override returns (uint256) { - return REVISION(); - } } diff --git a/contracts/src/v0.8/ccip/pools/GHO/VersionedInitializable.sol b/contracts/src/v0.8/ccip/pools/GHO/VersionedInitializable.sol deleted file mode 100644 index b9fb054fa0..0000000000 --- a/contracts/src/v0.8/ccip/pools/GHO/VersionedInitializable.sol +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0 -pragma solidity ^0.8.0; - -/** - * @title VersionedInitializable - * @author Aave, inspired by the OpenZeppelin Initializable contract - * @notice Helper contract to implement initializer functions. To use it, replace - * the constructor with a function that has the `initializer` modifier. - * @dev WARNING: Unlike constructors, initializer functions must be manually - * invoked. This applies both to deploying an Initializable contract, as well - * as extending an Initializable contract via inheritance. - * WARNING: When used with inheritance, manual care must be taken to not invoke - * a parent initializer twice, or ensure that all initializers are idempotent, - * because this is not dealt with automatically as with constructors. - */ -abstract contract VersionedInitializable { - /** - * @dev Indicates that the contract has been initialized. - */ - uint256 private lastInitializedRevision = 0; - - /** - * @dev Indicates that the contract is in the process of being initialized. - */ - bool private initializing; - - /** - * @dev Modifier to use in the initializer function of a contract. - */ - modifier initializer() { - uint256 revision = getRevision(); - require( - initializing || isConstructor() || revision > lastInitializedRevision, - "Contract instance has already been initialized" - ); - - bool isTopLevelCall = !initializing; - if (isTopLevelCall) { - initializing = true; - lastInitializedRevision = revision; - } - - _; - - if (isTopLevelCall) { - initializing = false; - } - } - - /** - * @notice Returns the revision number of the contract - * @dev Needs to be defined in the inherited class as a constant. - * @return The revision number - */ - function getRevision() internal pure virtual returns (uint256); - - /** - * @notice Returns true if and only if the function is running in the constructor - * @return True if the function is running in the constructor - */ - function isConstructor() private view returns (bool) { - // extcodesize checks the size of the code stored in an address, and - // address returns the current address. Since the code is still not - // deployed when running a constructor, any checks on its code size will - // yield zero, making it an effective way to detect if a contract is - // under construction or not. - uint256 cs; - //solium-disable-next-line - assembly { - cs := extcodesize(address()) - } - return cs == 0; - } - - // Reserved storage space to allow for layout changes in the future. - uint256[50] private ______gap; -} diff --git a/contracts/src/v0.8/ccip/pools/GHO/diffs/BurnMintTokenPoolAbstract_diff.md b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableBurnMintTokenPoolAbstract_diff.md similarity index 68% rename from contracts/src/v0.8/ccip/pools/GHO/diffs/BurnMintTokenPoolAbstract_diff.md rename to contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableBurnMintTokenPoolAbstract_diff.md index 11c20c0a6d..2255b2ca44 100644 --- a/contracts/src/v0.8/ccip/pools/GHO/diffs/BurnMintTokenPoolAbstract_diff.md +++ b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableBurnMintTokenPoolAbstract_diff.md @@ -1,18 +1,19 @@ ```diff -diff --git a/src/v0.8/ccip/pools/BurnMintTokenPoolAbstract.sol b/src/v0.8/ccip/pools/UpgradeableBurnMintTokenPoolAbstract.sol -index f5eb135186..651965e40b 100644 +diff --git a/src/v0.8/ccip/pools/BurnMintTokenPoolAbstract.sol b/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPoolAbstract.sol +index f5eb135186..e228732855 100644 --- a/src/v0.8/ccip/pools/BurnMintTokenPoolAbstract.sol -+++ b/src/v0.8/ccip/pools/UpgradeableBurnMintTokenPoolAbstract.sol ++++ b/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPoolAbstract.sol @@ -1,11 +1,11 @@ // SPDX-License-Identifier: BUSL-1.1 -pragma solidity 0.8.19; +pragma solidity ^0.8.0; - - import {IBurnMintERC20} from "../../shared/token/ERC20/IBurnMintERC20.sol"; - + +-import {IBurnMintERC20} from "../../shared/token/ERC20/IBurnMintERC20.sol"; ++import {IBurnMintERC20} from "../../../shared/token/ERC20/IBurnMintERC20.sol"; + -import {TokenPool} from "./TokenPool.sol"; +import {UpgradeableTokenPool} from "./UpgradeableTokenPool.sol"; - + -abstract contract BurnMintTokenPoolAbstract is TokenPool { +abstract contract UpgradeableBurnMintTokenPoolAbstract is UpgradeableTokenPool { /// @notice Contains the specific burn call for a pool. diff --git a/contracts/src/v0.8/ccip/pools/GHO/diffs/BurnMintTokenPool_diff.md b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableBurnMintTokenPool_diff.md similarity index 76% rename from contracts/src/v0.8/ccip/pools/GHO/diffs/BurnMintTokenPool_diff.md rename to contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableBurnMintTokenPool_diff.md index 1dfabb1e60..05c3be4d06 100644 --- a/contracts/src/v0.8/ccip/pools/GHO/diffs/BurnMintTokenPool_diff.md +++ b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableBurnMintTokenPool_diff.md @@ -1,39 +1,42 @@ ```diff -diff --git a/src/v0.8/ccip/pools/BurnMintTokenPool.sol b/src/v0.8/ccip/pools/UpgradeableBurnMintTokenPool.sol -index 9af0f22f4c..f07f8c3a28 100644 +diff --git a/src/v0.8/ccip/pools/BurnMintTokenPool.sol b/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPool.sol +index 9af0f22f4c..58be87812f 100644 --- a/src/v0.8/ccip/pools/BurnMintTokenPool.sol -+++ b/src/v0.8/ccip/pools/UpgradeableBurnMintTokenPool.sol -@@ -1,29 +1,66 @@ ++++ b/src/v0.8/ccip/pools/GHO/UpgradeableBurnMintTokenPool.sol +@@ -1,28 +1,55 @@ // SPDX-License-Identifier: BUSL-1.1 -pragma solidity 0.8.19; +pragma solidity ^0.8.0; - - import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; - import {IBurnMintERC20} from "../../shared/token/ERC20/IBurnMintERC20.sol"; - + +-import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; +-import {IBurnMintERC20} from "../../shared/token/ERC20/IBurnMintERC20.sol"; ++import {Initializable} from "solidity-utils/contracts/transparent-proxy/Initializable.sol"; + -import {TokenPool} from "./TokenPool.sol"; -import {BurnMintTokenPoolAbstract} from "./BurnMintTokenPoolAbstract.sol"; -+import {UpgradeableTokenPool} from "./UpgradeableTokenPool.sol"; -+import {UpgradeableBurnMintTokenPoolAbstract} from "./UpgradeableBurnMintTokenPoolAbstract.sol"; - ++import {ITypeAndVersion} from "../../../shared/interfaces/ITypeAndVersion.sol"; ++import {IBurnMintERC20} from "../../../shared/token/ERC20/IBurnMintERC20.sol"; + -/// @notice This pool mints and burns a 3rd-party token. -/// @dev Pool whitelisting mode is set in the constructor and cannot be modified later. -/// It either accepts any address as originalSender, or only accepts whitelisted originalSender. -/// The only way to change whitelisting mode is to deploy a new pool. -/// If that is expected, please make sure the token's burner/minter roles are adjustable. -contract BurnMintTokenPool is BurnMintTokenPoolAbstract, ITypeAndVersion { -+import {IRouter} from "../interfaces/IRouter.sol"; -+import {VersionedInitializable} from "./VersionedInitializable.sol"; ++import {UpgradeableTokenPool} from "./UpgradeableTokenPool.sol"; ++import {UpgradeableBurnMintTokenPoolAbstract} from "./UpgradeableBurnMintTokenPoolAbstract.sol"; ++ ++import {IRouter} from "../../interfaces/IRouter.sol"; + +/// @title UpgradeableBurnMintTokenPool +/// @author Aave Labs +/// @notice Upgradeable version of Chainlink's CCIP BurnMintTokenPool +/// @dev Contract adaptations: -+/// - Implementation of VersionedInitializable to allow upgrades ++/// - Implementation of Initializable to allow upgrades +/// - Move of allowlist and router definition to initialization stage -+contract UpgradeableBurnMintTokenPool is VersionedInitializable, UpgradeableBurnMintTokenPoolAbstract, ITypeAndVersion { ++contract UpgradeableBurnMintTokenPool is Initializable, UpgradeableBurnMintTokenPoolAbstract, ITypeAndVersion { string public constant override typeAndVersion = "BurnMintTokenPool 1.4.0"; - + + /// @dev Constructor + /// @param token The bridgeable token that is managed by this pool. + /// @param armProxy The address of the arm proxy @@ -47,7 +50,7 @@ index 9af0f22f4c..f07f8c3a28 100644 - ) TokenPool(token, allowlist, armProxy, router) {} + bool allowlistEnabled + ) UpgradeableTokenPool(IBurnMintERC20(token), armProxy, allowlistEnabled) {} - + - /// @inheritdoc BurnMintTokenPoolAbstract + /// @dev Initializer + /// @dev The address passed as `owner` must accept ownership after initialization. @@ -72,16 +75,4 @@ index 9af0f22f4c..f07f8c3a28 100644 function _burn(uint256 amount) internal virtual override { IBurnMintERC20(address(i_token)).burn(amount); } -+ -+ /// @notice Returns the revision number -+ /// @return The revision number -+ function REVISION() public pure virtual returns (uint256) { -+ return 1; -+ } -+ -+ /// @inheritdoc VersionedInitializable -+ function getRevision() internal pure virtual override returns (uint256) { -+ return REVISION(); -+ } - } ``` diff --git a/contracts/src/v0.8/ccip/pools/GHO/diffs/LockReleaseTokenPool_diff.md b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableLockReleaseTokenPool_diff.md similarity index 80% rename from contracts/src/v0.8/ccip/pools/GHO/diffs/LockReleaseTokenPool_diff.md rename to contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableLockReleaseTokenPool_diff.md index ac5d7bf30e..1e738e3bdf 100644 --- a/contracts/src/v0.8/ccip/pools/GHO/diffs/LockReleaseTokenPool_diff.md +++ b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableLockReleaseTokenPool_diff.md @@ -1,61 +1,64 @@ ```diff -diff --git a/src/v0.8/ccip/pools/LockReleaseTokenPool.sol b/src/v0.8/ccip/pools/UpgradeableLockReleaseTokenPool.sol -index 1a17fa0398..7ca3d5f389 100644 +diff --git a/src/v0.8/ccip/pools/LockReleaseTokenPool.sol b/src/v0.8/ccip/pools/GHO/UpgradeableLockReleaseTokenPool.sol +index 1a17fa0398..9a30b1e977 100644 --- a/src/v0.8/ccip/pools/LockReleaseTokenPool.sol -+++ b/src/v0.8/ccip/pools/UpgradeableLockReleaseTokenPool.sol -@@ -1,26 +1,41 @@ ++++ b/src/v0.8/ccip/pools/GHO/UpgradeableLockReleaseTokenPool.sol +@@ -1,26 +1,39 @@ // SPDX-License-Identifier: BUSL-1.1 -pragma solidity 0.8.19; +pragma solidity ^0.8.0; - - import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; - import {ILiquidityContainer} from "../../rebalancer/interfaces/ILiquidityContainer.sol"; - + +-import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; +-import {ILiquidityContainer} from "../../rebalancer/interfaces/ILiquidityContainer.sol"; ++import {Initializable} from "solidity-utils/contracts/transparent-proxy/Initializable.sol"; + -import {TokenPool} from "./TokenPool.sol"; +-import {RateLimiter} from "../libraries/RateLimiter.sol"; ++import {ITypeAndVersion} from "../../../shared/interfaces/ITypeAndVersion.sol"; ++import {ILiquidityContainer} from "../../../rebalancer/interfaces/ILiquidityContainer.sol"; + +-import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; +-import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol"; +import {UpgradeableTokenPool} from "./UpgradeableTokenPool.sol"; - import {RateLimiter} from "../libraries/RateLimiter.sol"; - - import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; - import {SafeERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol"; - ++import {RateLimiter} from "../../libraries/RateLimiter.sol"; + -/// @notice Token pool used for tokens on their native chain. This uses a lock and release mechanism. -/// Because of lock/unlock requiring liquidity, this pool contract also has function to add and remove -/// liquidity. This allows for proper bookkeeping for both user and liquidity provider balances. -/// @dev One token per LockReleaseTokenPool. -contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion { -+import {IRouter} from "../interfaces/IRouter.sol"; -+import {VersionedInitializable} from "./VersionedInitializable.sol"; ++import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; ++import {SafeERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/utils/SafeERC20.sol"; ++ ++import {IRouter} from "../../interfaces/IRouter.sol"; + +/// @title UpgradeableLockReleaseTokenPool +/// @author Aave Labs +/// @notice Upgradeable version of Chainlink's CCIP LockReleaseTokenPool +/// @dev Contract adaptations: -+/// - Implementation of VersionedInitializable to allow upgrades ++/// - Implementation of Initializable to allow upgrades +/// - Move of allowlist and router definition to initialization stage +/// - Addition of a bridge limit to regulate the maximum amount of tokens that can be transferred out (burned/locked) -+contract UpgradeableLockReleaseTokenPool is -+ VersionedInitializable, -+ UpgradeableTokenPool, -+ ILiquidityContainer, -+ ITypeAndVersion -+{ ++contract UpgradeableLockReleaseTokenPool is Initializable, UpgradeableTokenPool, ILiquidityContainer, ITypeAndVersion { using SafeERC20 for IERC20; - + error InsufficientLiquidity(); error LiquidityNotAccepted(); error Unauthorized(address caller); - + + error BridgeLimitExceeded(uint256 bridgeLimit); + error NotEnoughBridgedAmount(); ++ + event BridgeLimitUpdated(uint256 oldBridgeLimit, uint256 newBridgeLimit); ++ event BridgeLimitAdminUpdated(address indexed oldAdmin, address indexed newAdmin); + string public constant override typeAndVersion = "LockReleaseTokenPool 1.4.0"; - + /// @dev The unique lock release pool flag to signal through EIP 165. -@@ -37,16 +52,55 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion +@@ -37,16 +50,55 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion /// @dev Can be address(0) if none is configured. address internal s_rateLimitAdmin; - + + /// @notice Maximum amount of tokens that can be bridged to other chains + uint256 private s_bridgeLimit; + /// @notice Amount of tokens bridged (transferred out) @@ -83,7 +86,7 @@ index 1a17fa0398..7ca3d5f389 100644 + ) UpgradeableTokenPool(IERC20(token), armProxy, allowlistEnabled) { i_acceptLiquidity = acceptLiquidity; } - + + /// @dev Initializer + /// @dev The address passed as `owner` must accept ownership after initialization. + /// @dev The `allowlist` is only effective if pool is set to access-controlled mode @@ -113,7 +116,7 @@ index 1a17fa0398..7ca3d5f389 100644 /// @notice Locks the token in the pool /// @param amount Amount to lock /// @dev The whenHealthy check is important to ensure that even if a ramp is compromised -@@ -66,6 +120,9 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion +@@ -66,6 +118,9 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion whenHealthy returns (bytes memory) { @@ -123,7 +126,7 @@ index 1a17fa0398..7ca3d5f389 100644 _consumeOutboundRateLimit(remoteChainSelector, amount); emit Locked(msg.sender, amount); return ""; -@@ -83,6 +140,11 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion +@@ -83,6 +138,11 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion uint64 remoteChainSelector, bytes memory ) external virtual override onlyOffRamp(remoteChainSelector) whenHealthy { @@ -135,10 +138,10 @@ index 1a17fa0398..7ca3d5f389 100644 _consumeInboundRateLimit(remoteChainSelector, amount); getToken().safeTransfer(receiver, amount); emit Released(msg.sender, receiver, amount); -@@ -120,11 +182,46 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion +@@ -120,11 +180,48 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion s_rateLimitAdmin = rateLimitAdmin; } - + + /// @notice Sets the bridge limit, the maximum amount of tokens that can be bridged out + /// @dev Only callable by the owner or the bridge limit admin. + /// @dev Bridge limit changes should be carefully managed, specially when reducing below the current bridged amount @@ -154,7 +157,9 @@ index 1a17fa0398..7ca3d5f389 100644 + /// @dev Only callable by the owner. + /// @param bridgeLimitAdmin The new bridge limit admin address. + function setBridgeLimitAdmin(address bridgeLimitAdmin) external onlyOwner { ++ address oldAdmin = s_bridgeLimitAdmin; + s_bridgeLimitAdmin = bridgeLimitAdmin; ++ emit BridgeLimitAdminUpdated(oldAdmin, bridgeLimitAdmin); + } + + /// @notice Gets the bridge limit @@ -173,7 +178,7 @@ index 1a17fa0398..7ca3d5f389 100644 function getRateLimitAdmin() external view returns (address) { return s_rateLimitAdmin; } - + + /// @notice Gets the bridge limiter admin address. + function getBridgeLimitAdmin() external view returns (address) { + return s_bridgeLimitAdmin; @@ -182,20 +187,4 @@ index 1a17fa0398..7ca3d5f389 100644 /// @notice Checks if the pool can accept liquidity. /// @return true if the pool can accept liquidity, false otherwise. function canAcceptLiquidity() external view returns (bool) { -@@ -166,4 +263,15 @@ contract LockReleaseTokenPool is TokenPool, ILiquidityContainer, ITypeAndVersion - - _setRateLimitConfig(remoteChainSelector, outboundConfig, inboundConfig); - } -+ -+ /// @notice Returns the revision number -+ /// @return The revision number -+ function REVISION() public pure virtual returns (uint256) { -+ return 1; -+ } -+ -+ /// @inheritdoc VersionedInitializable -+ function getRevision() internal pure virtual override returns (uint256) { -+ return REVISION(); -+ } - } ``` diff --git a/contracts/src/v0.8/ccip/pools/GHO/diffs/TokenPool_diff.md b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableTokenPool_diff.md similarity index 61% rename from contracts/src/v0.8/ccip/pools/GHO/diffs/TokenPool_diff.md rename to contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableTokenPool_diff.md index 6ff8893172..fcdc197580 100644 --- a/contracts/src/v0.8/ccip/pools/GHO/diffs/TokenPool_diff.md +++ b/contracts/src/v0.8/ccip/pools/GHO/diffs/UpgradeableTokenPool_diff.md @@ -1,16 +1,32 @@ ```diff -diff --git a/src/v0.8/ccip/pools/TokenPool.sol b/src/v0.8/ccip/pools/UpgradeableTokenPool.sol -index b3571bb449..fcd8948098 100644 +diff --git a/src/v0.8/ccip/pools/TokenPool.sol b/src/v0.8/ccip/pools/GHO/UpgradeableTokenPool.sol +index b3571bb449..ee359ac1f8 100644 --- a/src/v0.8/ccip/pools/TokenPool.sol -+++ b/src/v0.8/ccip/pools/UpgradeableTokenPool.sol -@@ -1,5 +1,5 @@ ++++ b/src/v0.8/ccip/pools/GHO/UpgradeableTokenPool.sol +@@ -1,21 +1,21 @@ // SPDX-License-Identifier: BUSL-1.1 -pragma solidity 0.8.19; +pragma solidity ^0.8.0; - - import {IPool} from "../interfaces/pools/IPool.sol"; - import {IARM} from "../interfaces/IARM.sol"; -@@ -15,7 +15,7 @@ import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts + +-import {IPool} from "../interfaces/pools/IPool.sol"; +-import {IARM} from "../interfaces/IARM.sol"; +-import {IRouter} from "../interfaces/IRouter.sol"; ++import {IPool} from "../../interfaces/pools/IPool.sol"; ++import {IARM} from "../../interfaces/IARM.sol"; ++import {IRouter} from "../../interfaces/IRouter.sol"; + +-import {OwnerIsCreator} from "../../shared/access/OwnerIsCreator.sol"; +-import {RateLimiter} from "../libraries/RateLimiter.sol"; ++import {OwnerIsCreator} from "../../../shared/access/OwnerIsCreator.sol"; ++import {RateLimiter} from "../../libraries/RateLimiter.sol"; + +-import {IERC20} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; +-import {IERC165} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/introspection/IERC165.sol"; +-import {EnumerableSet} from "../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableSet.sol"; ++import {IERC20} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/token/ERC20/IERC20.sol"; ++import {IERC165} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/introspection/IERC165.sol"; ++import {EnumerableSet} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/utils/structs/EnumerableSet.sol"; + /// @notice Base abstract class with common functions for all token pools. /// A token pool serves as isolated place for holding tokens and token specific logic /// that may execute as tokens move across the bridge. @@ -30,7 +46,7 @@ index b3571bb449..fcd8948098 100644 /// degrees and prefer different limits) - mapping(uint64 remoteChainSelector => RateLimiter.TokenBucket) internal s_inboundRateLimits; + mapping(uint64 => RateLimiter.TokenBucket) internal s_inboundRateLimits; - + - constructor(IERC20 token, address[] memory allowlist, address armProxy, address router) { - if (address(token) == address(0) || router == address(0)) revert ZeroAddressNotAllowed(); + constructor(IERC20 token, address armProxy, bool allowlistEnabled) { @@ -46,6 +62,6 @@ index b3571bb449..fcd8948098 100644 - } + i_allowlistEnabled = allowlistEnabled; } - + /// @notice Get ARM proxy address ``` diff --git a/contracts/src/v0.8/ccip/test/mocks/MockUpgradeable.sol b/contracts/src/v0.8/ccip/test/mocks/MockUpgradeable.sol index bd80dee812..45eeb5c5d7 100644 --- a/contracts/src/v0.8/ccip/test/mocks/MockUpgradeable.sol +++ b/contracts/src/v0.8/ccip/test/mocks/MockUpgradeable.sol @@ -1,12 +1,12 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -import {VersionedInitializable} from "../../pools/GHO/VersionedInitializable.sol"; +import {Initializable} from "solidity-utils/contracts/transparent-proxy/Initializable.sol"; /** * @dev Mock contract to test upgrades, not to be used in production. */ -contract MockUpgradeable is VersionedInitializable { +contract MockUpgradeable is Initializable { /** * @dev Constructor */ @@ -17,20 +17,7 @@ contract MockUpgradeable is VersionedInitializable { /** * @dev Initializer */ - function initialize() public initializer { + function initialize() public reinitializer(2) { // Intentionally left bank } - - /** - * @notice Returns the revision number - * @return The revision number - */ - function REVISION() public pure returns (uint256) { - return 2; - } - - /// @inheritdoc VersionedInitializable - function getRevision() internal pure virtual override returns (uint256) { - return REVISION(); - } } diff --git a/contracts/src/v0.8/ccip/test/pools/GHO/GhoBaseTest.t.sol b/contracts/src/v0.8/ccip/test/pools/GHO/GhoBaseTest.t.sol index f14e67fff9..66d6fc63b5 100644 --- a/contracts/src/v0.8/ccip/test/pools/GHO/GhoBaseTest.t.sol +++ b/contracts/src/v0.8/ccip/test/pools/GHO/GhoBaseTest.t.sol @@ -43,10 +43,8 @@ abstract contract GhoBaseTest is BaseTest { ) internal returns (address) { // Deploy BurnMintTokenPool for GHO token on source chain UpgradeableBurnMintTokenPool tokenPoolImpl = new UpgradeableBurnMintTokenPool(ghoToken, arm, false); - // Imple init - address[] memory emptyArray = new address[](0); - tokenPoolImpl.initialize(owner, emptyArray, router); // proxy deploy and init + address[] memory emptyArray = new address[](0); bytes memory tokenPoolInitParams = abi.encodeWithSignature( "initialize(address,address[],address)", owner, @@ -76,10 +74,8 @@ abstract contract GhoBaseTest is BaseTest { address proxyAdmin ) internal returns (address) { UpgradeableLockReleaseTokenPool tokenPoolImpl = new UpgradeableLockReleaseTokenPool(ghoToken, arm, false, true); - // Imple init - address[] memory emptyArray = new address[](0); - tokenPoolImpl.initialize(owner, emptyArray, router, bridgeLimit); // proxy deploy and init + address[] memory emptyArray = new address[](0); bytes memory tokenPoolInitParams = abi.encodeWithSignature( "initialize(address,address[],address,uint256)", owner, @@ -120,6 +116,11 @@ abstract contract GhoBaseTest is BaseTest { return address(uint160(uint256(implSlot))); } + function _getUpgradeableVersion(address proxy) internal view returns (uint8) { + // version is 1st slot + return uint8(uint256(vm.load(proxy, bytes32(uint256(0))))); + } + function _enableLane(UtilsStorage storage s, uint256 fromId, uint256 toId) internal { // from UpgradeableTokenPool.ChainUpdate[] memory chainUpdate = new UpgradeableTokenPool.ChainUpdate[](1); diff --git a/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolEthereum.t.sol b/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolEthereum.t.sol index 46da97244c..f2824d7b6f 100644 --- a/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolEthereum.t.sol +++ b/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolEthereum.t.sol @@ -477,6 +477,7 @@ contract GhoTokenPoolEthereum_setRateLimitAdmin is GhoTokenPoolEthereumSetup { contract GhoTokenPoolEthereum_setBridgeLimit is GhoTokenPoolEthereumSetup { event BridgeLimitUpdated(uint256 oldBridgeLimit, uint256 newBridgeLimit); + event BridgeLimitAdminUpdated(address indexed oldAdmin, address indexed newAdmin); function testSetBridgeLimitAdminSuccess() public { assertEq(INITIAL_BRIDGE_LIMIT, s_ghoTokenPool.getBridgeLimit()); @@ -493,6 +494,10 @@ contract GhoTokenPoolEthereum_setBridgeLimit is GhoTokenPoolEthereumSetup { // Bridge Limit Admin address bridgeLimitAdmin = address(28973509103597907); + + vm.expectEmit(); + emit BridgeLimitAdminUpdated(address(0), bridgeLimitAdmin); + s_ghoTokenPool.setBridgeLimitAdmin(bridgeLimitAdmin); vm.startPrank(bridgeLimitAdmin); @@ -593,11 +598,17 @@ contract GhoTokenPoolEthereum_setBridgeLimit is GhoTokenPoolEthereumSetup { } contract GhoTokenPoolEthereum_setBridgeLimitAdmin is GhoTokenPoolEthereumSetup { + event BridgeLimitAdminUpdated(address indexed oldAdmin, address indexed newAdmin); + function testSetBridgeLimitAdminSuccess() public { assertEq(address(0), s_ghoTokenPool.getBridgeLimitAdmin()); address bridgeLimitAdmin = address(28973509103597907); changePrank(AAVE_DAO); + + vm.expectEmit(); + emit BridgeLimitAdminUpdated(address(0), bridgeLimitAdmin); + s_ghoTokenPool.setBridgeLimitAdmin(bridgeLimitAdmin); assertEq(bridgeLimitAdmin, s_ghoTokenPool.getBridgeLimitAdmin()); @@ -616,7 +627,7 @@ contract GhoTokenPoolEthereum_setBridgeLimitAdmin is GhoTokenPoolEthereumSetup { contract GhoTokenPoolEthereum_upgradeability is GhoTokenPoolEthereumSetup { function testInitialization() public { // Upgradeability - assertEq(s_ghoTokenPool.REVISION(), 1); + assertEq(_getUpgradeableVersion(address(s_ghoTokenPool)), 1); vm.startPrank(PROXY_ADMIN); (bool ok, bytes memory result) = address(s_ghoTokenPool).staticcall( abi.encodeWithSelector(TransparentUpgradeableProxy.admin.selector) @@ -643,17 +654,17 @@ contract GhoTokenPoolEthereum_upgradeability is GhoTokenPoolEthereumSetup { TransparentUpgradeableProxy(payable(address(s_ghoTokenPool))).upgradeToAndCall(address(newImpl), mockImpleParams); vm.startPrank(OWNER); - assertEq(s_ghoTokenPool.REVISION(), 2); + assertEq(_getUpgradeableVersion(address(s_ghoTokenPool)), 2); } function testUpgradeAdminReverts() public { vm.expectRevert(); TransparentUpgradeableProxy(payable(address(s_ghoTokenPool))).upgradeToAndCall(address(0), bytes("")); - assertEq(s_ghoTokenPool.REVISION(), 1); + assertEq(_getUpgradeableVersion(address(s_ghoTokenPool)), 1); vm.expectRevert(); TransparentUpgradeableProxy(payable(address(s_ghoTokenPool))).upgradeTo(address(0)); - assertEq(s_ghoTokenPool.REVISION(), 1); + assertEq(_getUpgradeableVersion(address(s_ghoTokenPool)), 1); } function testChangeAdmin() public { diff --git a/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolRemote.t.sol b/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolRemote.t.sol index bba33d8066..dd784e68c1 100644 --- a/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolRemote.t.sol +++ b/contracts/src/v0.8/ccip/test/pools/GHO/GhoTokenPoolRemote.t.sol @@ -182,7 +182,7 @@ contract GhoTokenPoolRemote_releaseOrMint is GhoTokenPoolRemoteSetup { contract GhoTokenPoolEthereum_upgradeability is GhoTokenPoolRemoteSetup { function testInitialization() public { // Upgradeability - assertEq(s_pool.REVISION(), 1); + assertEq(_getUpgradeableVersion(address(s_pool)), 1); vm.startPrank(PROXY_ADMIN); (bool ok, bytes memory result) = address(s_pool).staticcall( abi.encodeWithSelector(TransparentUpgradeableProxy.admin.selector) @@ -209,17 +209,17 @@ contract GhoTokenPoolEthereum_upgradeability is GhoTokenPoolRemoteSetup { TransparentUpgradeableProxy(payable(address(s_pool))).upgradeToAndCall(address(newImpl), mockImpleParams); vm.startPrank(OWNER); - assertEq(s_pool.REVISION(), 2); + assertEq(_getUpgradeableVersion(address(s_pool)), 2); } function testUpgradeAdminReverts() public { vm.expectRevert(); TransparentUpgradeableProxy(payable(address(s_pool))).upgradeToAndCall(address(0), bytes("")); - assertEq(s_pool.REVISION(), 1); + assertEq(_getUpgradeableVersion(address(s_pool)), 1); vm.expectRevert(); TransparentUpgradeableProxy(payable(address(s_pool))).upgradeTo(address(0)); - assertEq(s_pool.REVISION(), 1); + assertEq(_getUpgradeableVersion(address(s_pool)), 1); } function testChangeAdmin() public { diff --git a/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolEthereumBridgeLimitInvariant.t.sol b/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolEthereumBridgeLimitInvariant.t.sol index 4d928e5fba..2b730bab1e 100644 --- a/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolEthereumBridgeLimitInvariant.t.sol +++ b/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolEthereumBridgeLimitInvariant.t.sol @@ -17,6 +17,10 @@ contract GhoTokenPoolEthereumBridgeLimitInvariant is BaseTest { deal(handler.tokens(0), address(handler), handler.INITIAL_BRIDGE_LIMIT()); targetContract(address(handler)); + bytes4[] memory selectors = new bytes4[](2); + selectors[0] = GhoTokenPoolHandler.bridgeGho.selector; + selectors[1] = GhoTokenPoolHandler.updateBucketCapacity.selector; + targetSelector(FuzzSelector({addr: address(handler), selectors: selectors})); } /// forge-config: ccip.invariant.fail-on-revert = true diff --git a/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolHandler.t.sol b/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolHandler.t.sol index 3c4e0e951f..2b3491a7d4 100644 --- a/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolHandler.t.sol +++ b/contracts/src/v0.8/ccip/test/pools/GHO/invariant/GhoTokenPoolHandler.t.sol @@ -45,14 +45,15 @@ contract GhoTokenPoolHandler is GhoBaseTest { function bridgeGho(uint256 fromChain, uint256 toChain, uint256 amount) public { fromChain = bound(fromChain, 0, 2); toChain = bound(toChain, 0, 2); - vm.assume(fromChain != toChain); - uint256 maxBalance = GhoToken(s.tokens[fromChain]).balanceOf(address(this)); - uint256 maxToBridge = _getMaxToBridgeOut(s, fromChain); - uint256 maxAmount = maxBalance > maxToBridge ? maxToBridge : maxBalance; - amount = bound(amount, 0, maxAmount); - - if (amount > 0) { - _bridgeGho(s, fromChain, toChain, address(this), amount); + if (fromChain != toChain) { + uint256 maxBalance = GhoToken(s.tokens[fromChain]).balanceOf(address(this)); + uint256 maxToBridge = _getMaxToBridgeOut(s, fromChain); + uint256 maxAmount = maxBalance > maxToBridge ? maxToBridge : maxBalance; + amount = bound(amount, 0, maxAmount); + + if (amount > 0) { + _bridgeGho(s, fromChain, toChain, address(this), amount); + } } } @@ -60,7 +61,7 @@ contract GhoTokenPoolHandler is GhoBaseTest { function updateBucketCapacity(uint256 chain, uint128 newCapacity) public { chain = bound(chain, 1, 2); uint256 otherChain = (chain % 2) + 1; - vm.assume(newCapacity >= s.bridged); + newCapacity = uint128(bound(newCapacity, s.bridged, type(uint128).max)); uint256 oldCapacity = s.bucketCapacities[chain];