diff --git a/pkg/balancer-js/src/utils/errors.ts b/pkg/balancer-js/src/utils/errors.ts index 6202451d0d..0b0da1b07e 100644 --- a/pkg/balancer-js/src/utils/errors.ts +++ b/pkg/balancer-js/src/utils/errors.ts @@ -14,6 +14,7 @@ const balancerErrorCodes: Record = { '102': 'UNSORTED_TOKENS', '103': 'INPUT_LENGTH_MISMATCH', '104': 'ZERO_TOKEN', + '105': 'INSUFFICIENT_DATA', '200': 'MIN_TOKENS', '201': 'MAX_TOKENS', '202': 'MAX_SWAP_FEE_PERCENTAGE', diff --git a/pkg/interfaces/CHANGELOG.md b/pkg/interfaces/CHANGELOG.md index ad426e6822..5a462afc66 100644 --- a/pkg/interfaces/CHANGELOG.md +++ b/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) diff --git a/pkg/interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol b/pkg/interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol index 8eac914d55..46a633bad0 100644 --- a/pkg/interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol +++ b/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; diff --git a/pkg/liquidity-mining/contracts/admin/AuthorizerAdaptor.sol b/pkg/liquidity-mining/contracts/admin/AuthorizerAdaptor.sol index 0040ffef0c..9e4f2a20b5 100644 --- a/pkg/liquidity-mining/contracts/admin/AuthorizerAdaptor.sol +++ b/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. diff --git a/pkg/liquidity-mining/contracts/admin/AuthorizerAdaptorEntrypoint.sol b/pkg/liquidity-mining/contracts/admin/AuthorizerAdaptorEntrypoint.sol index 3424f0ba06..76d3ad08b8 100644 --- a/pkg/liquidity-mining/contracts/admin/AuthorizerAdaptorEntrypoint.sol +++ b/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); } } diff --git a/pkg/liquidity-mining/test/AuthorizerAdaptorEntrypoint.test.ts b/pkg/liquidity-mining/test/AuthorizerAdaptorEntrypoint.test.ts index 2908920d68..ad8176134b 100644 --- a/pkg/liquidity-mining/test/AuthorizerAdaptorEntrypoint.test.ts +++ b/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' + ); }); }); }); diff --git a/pkg/vault/contracts/authorizer/TimelockAuthorizer.sol b/pkg/vault/contracts/authorizer/TimelockAuthorizer.sol index ba30322095..465f5fb93a 100644 --- a/pkg/vault/contracts/authorizer/TimelockAuthorizer.sol +++ b/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); }