Merge branch 'master' into external-call-wrapper

* master:
  Fix linter (#2006)
  Improve AuthorizerAdaptorEntrypoint docs (#2003)
  Changelog update (#2000)

pkg/balancer-js/src/utils/errors.ts

@@ -14,6 +14,7 @@ const balancerErrorCodes: Record<string, string> = {
   '102': 'UNSORTED_TOKENS',
   '103': 'INPUT_LENGTH_MISMATCH',
   '104': 'ZERO_TOKEN',
+  '105': 'INSUFFICIENT_DATA',
   '200': 'MIN_TOKENS',
   '201': 'MAX_TOKENS',
   '202': 'MAX_SWAP_FEE_PERCENTAGE',

pkg/interfaces/CHANGELOG.md

@@ -6,6 +6,7 @@
 
 - Added `IProtocolFeeSplitter`.
 - Added `IL2GaugeCheckpointer`.
+- Added `IAuthorizerAdaptorEntrypoint`.
 
 ### New Features
 
@@ -14,6 +15,7 @@
 ### Breaking Changes
 
 - Removed `IAssetManager`, which was unused.
+- `IGaugeAdder`: authorizer adaptor getter replaced with authorizer adaptor entrypoint getter.
 
 ## 0.1.0 (2022-10-25)
 

pkg/interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol

@@ -123,6 +123,7 @@ library Errors {
     uint256 internal constant UNSORTED_TOKENS = 102;
     uint256 internal constant INPUT_LENGTH_MISMATCH = 103;
     uint256 internal constant ZERO_TOKEN = 104;
+    uint256 internal constant INSUFFICIENT_DATA = 105;
 
     // Shared pools
     uint256 internal constant MIN_TOKENS = 200;

pkg/liquidity-mining/contracts/admin/AuthorizerAdaptor.sol

@@ -23,6 +23,13 @@ import "@balancer-labs/v2-solidity-utils/contracts/openzeppelin/Address.sol";
 
 /**
  * @title Authorizer Adaptor
+ *
+ * WARNING: this contract contains a *critical bug* that can lead into exploits where it checks for permissions
+ * incorrectly. It should *never* be used by itself. We keep a copy of it in the repository, including the bug and all
+ * original comments (some of which are incorrect due to the bug), both for historical reasons and because it is part of
+ * our immutable infrastructure. See the `AuthorizerAdaptorEntrypoint` contract for more information on how we use this
+ * contract safely.
+ *
  * @notice This contract is intended to act as an adaptor between systems which expect a single admin address
  * and the Balancer Authorizer such that the Authorizer may grant/revoke admin powers to unlimited addresses.
  *
@@ -80,7 +87,9 @@ contract AuthorizerAdaptor is IAuthorizerAdaptor, ReentrancyGuard {
 
     /**
      * @notice Performs an arbitrary function call on a target contract, provided the caller is authorized to do so.
-     * @dev This function shall not be called directly; see `performAction` in `AuthorizerAdaptorEntrypoint`.
+     *
+     * This function should not be called directly as that will result in an unconditional revert: instead, use
+     * `AuthorizerAdaptorEntrypoint.performAction`.
      * @param target - Address of the contract to be called
      * @param data - Calldata to be sent to the target contract
      * @return The bytes encoded return value from the performed function call
@@ -92,6 +101,31 @@ contract AuthorizerAdaptor is IAuthorizerAdaptor, ReentrancyGuard {
         nonReentrant
         returns (bytes memory)
     {
+        // WARNING: the following line contains a critical bug that allows the caller to trick this contract into
+        // checking for an incorrect permission.
+        // We unconditionally read memory slot 100, which is where the first four bytes of `data` will reside (i.e. the
+        // function selector) given a standard packed ABI encoding. Both the Solidity compiler and clients such as
+        // ethers.js will do the ABI encoding in such a way that the selector is actually on slot 100, since this is the
+        // way that minimizes gas costs, but it is *not* the only valid way to ABI encode.
+        // In particular, it is possible to choose a larger offset and place `data` much further away in calldata. Under
+        // those conditions, slot 100 will *not* contain the selector, but it can instead be any arbitrary value. This
+        // means that the AuthorizerAdaptor can be made to check for the permission of any arbitrary selector,
+        // regardless of the action encoded in `data`.
+        //
+        // In other words, an account that has permission to execute *any* action via the Adaptor can actually execute
+        // *all* of them: there's no permission granularity.
+        // Note that actually performing this exploit requires the ability to manually craft calldata: as such,
+        // Solidity contracts that call into the Adaptor and create the call via the `abi.encode` function are safe to
+        // use since they will always use the standard encoding.
+        //
+        // To work around this issue, the `TimelockAuthorizer` contract contains a special condition that will check
+        // when it is being called by the `AuthorizerAdaptor`, and behave differently when that happens. See the
+        // `TimelockAuthorizer.canPerform` and `AuthorizerAdaptorEntrypoint.performAction` functions for more
+        // information.
+        //
+        // All comments below are part of the original source code, and as noted above some of them are incorrect. They
+        // are kept for historical reasons.
+
         bytes4 selector;
 
         // We want to check that the caller is authorized to call the function on the target rather than this function.
@@ -108,7 +142,9 @@ contract AuthorizerAdaptor is IAuthorizerAdaptor, ReentrancyGuard {
             selector := calldataload(100)
         }
 
-        // This check should only pass if the sender is the authorizer adaptor entrypoint.
+        // NOTE: The `TimelockAuthorizer` special cases the `AuthorizerAdaptor` calling into it, so that the action ID
+        // and `target` values are completely ignored. The following check will only pass if the caller is the
+        // `AuthorizerAdaptorEntrypoint`, which will have already checked for permissions correctly.
         _require(_canPerform(getActionId(selector), msg.sender, target), Errors.SENDER_NOT_ALLOWED);
 
         // We don't check that `target` is a contract so all calls to an EOA will succeed.

pkg/liquidity-mining/contracts/admin/AuthorizerAdaptorEntrypoint.sol

@@ -23,12 +23,10 @@ import "@balancer-labs/v2-solidity-utils/contracts/openzeppelin/Address.sol";
 
 /**
  * @title Authorizer Adaptor Entrypoint
- * @notice This contract is intended to act as an entrypoint to perform actions via the authorizer adaptor, ensuring
- * actions are properly validated beforehand.
- *
- * @dev When calculating the actionId to call a function on a target contract, it must be calculated as if it were
- * to be called on the authorizer adaptor. This can be done by passing the function selector to the `getActionId`
- * function.
+ * @notice This contract exists as a fix for a critical bug in the `AuthorizerAdaptor` that could lead to escalation of
+ * privileges. The Entrypoint contract addresses this by working in combination with `TimelockAuthorizer` so that all
+ * Adaptor calls that are not made via the Entrypoint fail, while those that do happen through the Entrypoint check for
+ * permissions correctly.
  */
 contract AuthorizerAdaptorEntrypoint is IAuthorizerAdaptorEntrypoint {
     using Address for address;
@@ -41,23 +39,14 @@ contract AuthorizerAdaptorEntrypoint is IAuthorizerAdaptorEntrypoint {
         _vault = adaptor.getVault();
     }
 
-    /**
-     * @notice Returns the Balancer Vault
-     */
     function getVault() public view override returns (IVault) {
         return _vault;
     }
 
-    /**
-     * @notice Returns the Authorizer
-     */
     function getAuthorizer() public view override returns (IAuthorizer) {
         return getVault().getAuthorizer();
     }
 
-    /**
-     * @notice Returns the Authorizer Adaptor
-     */
     function getAuthorizerAdaptor() public view override returns (IAuthorizerAdaptor) {
         return _adaptor;
     }
@@ -71,39 +60,32 @@ contract AuthorizerAdaptorEntrypoint is IAuthorizerAdaptorEntrypoint {
     }
 
     /**
-     * @notice Returns the action ID associated with calling a given function through the authorizer adaptor.
-     * @dev As the contracts managed by the adaptor don't have action ID disambiguators, we use the adaptor's globally.
-     * This means that contracts with the same function selector will have a matching action ID:
-     * if granularity is required then permissions must not be granted globally in the Authorizer.
-     *
-     * The adaptor entrypoint does not hold a disambiguator of its own; this function just forwards the call to the
-     * adaptor itself.
+     * @notice Returns the action ID associated with calling a given function through the `AuthorizerAdaptor`. Note that
+     * even though the Adaptor's action IDs are not actually used by it (since the Authorizer ignores those values - see
+     * `TimelockAuthorizer.canPerform`), this contract reuses those IDs to simplify migrations and tooling.
      *
-     * @param selector - The 4 byte selector of the function to be called using `performAction`
-     * @return The associated action ID
+     * See `AuthorizerAdaptor.getActionId` for more information on how the action IDs are computed, and how functions
+     * with equal selectors are assigned the same action ID.
      */
     function getActionId(bytes4 selector) public view override returns (bytes32) {
         return getAuthorizerAdaptor().getActionId(selector);
     }
 
-    /**
-     * @notice Performs an arbitrary function call on a target contract, provided the caller is authorized to do so.
-     * @param target - Address of the contract to be called
-     * @param data - Calldata to be sent to the target contract. It should be at least 4 bytes long (i.e. the length of
-     * the selector corresponding to the function to be called)
-     * @return The bytes encoded return value from the performed function call
-     */
     function performAction(address target, bytes calldata data) external payable override returns (bytes memory) {
         // We want to check that the caller is authorized to call the function on the target rather than this function.
         // We must then pull the function selector from `data` rather than `msg.sig`.
-        // Note that if `data` is less than 4 bytes long this will revert.
+
+        // Note that this will revert if `data` is less than 4 bytes long. We test for that to provide a nicer revert
+        // reason.
+        _require(data.length >= 4, Errors.INSUFFICIENT_DATA);
         bytes4 selector = data[0] | (bytes4(data[1]) >> 8) | (bytes4(data[2]) >> 16) | (bytes4(data[3]) >> 24);
 
-        // This call to `canPerform` will validate the actual action ID and sender in the authorizer.
         _require(canPerform(getActionId(selector), msg.sender, target), Errors.SENDER_NOT_ALLOWED);
 
-        // Contracts using the adaptor expect it to be the caller of the actions to perform, so we forward
-        // the call to `performAction` to the adaptor instead of performing it directly.
+        // The `AuthorizerAdaptor` will not check for permissions: it is special-cased in the `TimelockAuthorizer` so
+        // that all calls to it that are not made from this entrypoint fail, while those that originate in the
+        // entrypoint succeed. This works as we have just checked that the caller has permission to perform the action
+        // encoded by `data`. See `TimelockAuthorizer.canPerform` for more details.
         return getAuthorizerAdaptor().performAction{ value: msg.value }(target, data);
     }
 }

pkg/liquidity-mining/test/AuthorizerAdaptorEntrypoint.test.ts

@@ -47,15 +47,15 @@ describe('AuthorizerAdaptorEntrypoint', () => {
       expect(await adaptorEntrypoint.getAuthorizer()).to.equal(authorizer.address);
     });
 
-    it('returns the same action ID as the adaptor', async () => {
-      expect(await adaptorEntrypoint.getActionId('0xaabbccdd')).to.equal(await adaptor.getActionId('0xaabbccdd'));
-    });
-
     it('tracks authorizer changes in the vault', async () => {
       await vault.setAuthorizer(other);
 
       expect(await adaptorEntrypoint.getAuthorizer()).to.equal(other.address);
     });
+
+    it('returns the same action ID as the adaptor', async () => {
+      expect(await adaptorEntrypoint.getActionId('0xaabbccdd')).to.equal(await adaptor.getActionId('0xaabbccdd'));
+    });
   });
 
   describe('performAction', () => {
@@ -147,7 +147,9 @@ describe('AuthorizerAdaptorEntrypoint', () => {
 
     context('when calldata is invalid', () => {
       it('reverts', async () => {
-        await expect(adaptorEntrypoint.connect(other).performAction(target, '0x')).to.be.reverted;
+        await expect(adaptorEntrypoint.connect(other).performAction(target, '0x')).to.be.revertedWith(
+          'INSUFFICIENT_DATA'
+        );
       });
     });
   });

pkg/vault/contracts/authorizer/TimelockAuthorizer.sol

@@ -366,14 +366,16 @@ contract TimelockAuthorizer is IAuthorizer, IAuthentication, ReentrancyGuard {
         address where
     ) public view override returns (bool) {
         if (msg.sender == address(_authorizerAdaptor)) {
-            // We special case the situation where the caller is the authorizer adaptor.
-            // We do this as it doesn't properly calculate the value of `actionId` to pass to the authorizer,
-            // potentially allowing addresses to perform privilege escalations.
+            // We special case the situation where the caller is the `AuthorizerAdaptor`, as it can be tricked into
+            // passing an incorrect `actionId` value, potentially resulting in escalation of privileges.
             //
-            // To remedy this we force all calls to the authorizer adaptor be through a singleton entrypoint contract
-            // This contract correctly checks whether `account` can perform `actionId` on `where` and forwards the call
-            // onto the authorizer adaptor to execute.
-            // The authorizer must then reject calls to the authorizer adaptor which aren't made through the entrypoint.
+            // To remedy this we force all calls to the `AuthorizerAdaptor` to be made through a singleton entrypoint
+            // contract, called the `AuthorizerAdaptorEntrypoint`. This contract correctly checks whether `account` can
+            // perform `actionId` on `where`, and then forwards the call onto the `AuthorizerAdaptor` to execute.
+            //
+            // The authorizer then rejects calls to the `AuthorizerAdaptor` which aren't made through the entrypoint,
+            // and approves all calls made through it (since the entrypoint will have already performed any necessary
+            // permission checks).
             return account == address(_authorizerAdaptorEntrypoint);
         }