feat: implementing forceApprove in _afterFinalize hook (#183)

* test: adding expectations on _afterFinalize test

* feat: adding forceApprove to BCoWPool::_afterFinalize

* chore: updating gas snapshots

.forge-snapshots/newBCoWFactory.snap

@@ -1 +1 @@
-4314884
+4360005

.forge-snapshots/newBCoWPool.snap

@@ -1 +1 @@
-3401020
+3442739

src/contracts/BCoWPool.sol

@@ -21,6 +21,7 @@ import {BPool} from './BPool.sol';
 import {GPv2Order} from '@cowprotocol/libraries/GPv2Order.sol';
 import {IERC1271} from '@openzeppelin/contracts/interfaces/IERC1271.sol';
 import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
+import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
 
 import {IBCoWFactory} from 'interfaces/IBCoWFactory.sol';
 import {IBCoWPool} from 'interfaces/IBCoWPool.sol';
@@ -33,6 +34,7 @@ import {ISettlement} from 'interfaces/ISettlement.sol';
  */
 contract BCoWPool is IERC1271, IBCoWPool, BPool, BCoWConst {
   using GPv2Order for GPv2Order.Data;
+  using SafeERC20 for IERC20;
 
   /// @inheritdoc IBCoWPool
   address public immutable VAULT_RELAYER;
@@ -146,7 +148,7 @@ contract BCoWPool is IERC1271, IBCoWPool, BPool, BCoWConst {
   function _afterFinalize() internal override {
     uint256 tokensLength = _tokens.length;
     for (uint256 i; i < tokensLength; i++) {
-      IERC20(_tokens[i]).approve(VAULT_RELAYER, type(uint256).max);
+      IERC20(_tokens[i]).forceApprove(VAULT_RELAYER, type(uint256).max);
     }
 
     // Make the factory emit the event, to be easily indexed by off-chain agents

test/unit/BCoWPool/BCoWPool.t.sol

@@ -3,6 +3,9 @@ pragma solidity 0.8.25;
 
 import {IERC20} from '@cowprotocol/interfaces/IERC20.sol';
 
+import {SafeERC20} from '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
+import {Address} from '@openzeppelin/contracts/utils/Address.sol';
+
 import {BCoWPoolBase} from './BCoWPoolBase.t.sol';
 
 import {IBCoWFactory} from 'interfaces/IBCoWFactory.sol';
@@ -53,6 +56,42 @@ contract BCoWPool is BCoWPoolBase {
     assertEq(pool.APP_DATA(), _appData);
   }
 
+  function test__afterFinalizeRevertWhen_OneOfTheTokensReturnsFalse() external {
+    vm.mockCall(
+      tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), abi.encode(false)
+    );
+    // it should revert
+    vm.expectRevert(abi.encodeWithSelector(SafeERC20.SafeERC20FailedOperation.selector, tokens[1]));
+    bCoWPool.call__afterFinalize();
+  }
+
+  function test__afterFinalizeWhenOneOfTheTokensDoesntReturnAValue() external {
+    vm.mockCall(
+      tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), abi.encode()
+    );
+    // it assumes approval success
+    bCoWPool.call__afterFinalize();
+  }
+
+  function test__afterFinalizeRevertWhen_OneOfTheTokensRevertsWithoutData() external {
+    vm.mockCallRevert(
+      tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), abi.encode()
+    );
+    // it should revert with FailedInnerCall
+    vm.expectRevert(Address.FailedInnerCall.selector);
+    bCoWPool.call__afterFinalize();
+  }
+
+  function test__afterFinalizeRevertWhen_OneOfTheTokensRevertsWithData(bytes memory errorData) external {
+    vm.assume(keccak256(errorData) != keccak256(bytes('')));
+    vm.mockCallRevert(
+      tokens[1], abi.encodeWithSelector(IERC20.approve.selector, vaultRelayer, type(uint256).max), errorData
+    );
+    // it should revert with same error data
+    vm.expectRevert(errorData);
+    bCoWPool.call__afterFinalize();
+  }
+
   function test__afterFinalizeWhenCalled() external {
     // it calls approve on every bound token
     vm.expectCall(tokens[0], abi.encodeCall(IERC20.approve, (vaultRelayer, type(uint256).max)));

test/unit/BCoWPool/BCoWPool.tree

@@ -8,6 +8,14 @@ BCoWPool::constructor
     └── it should set the app data
 
 BCoWPool::_afterFinalize
+├── when one of the tokens returns false
+│   └── it should revert
+├── when one of the tokens doesnt return a value
+│   └── it assumes approval success
+├── when one of the tokens reverts without data
+│   └── it should revert // FailedInnerCall
+├── when one of the tokens reverts with data
+│   └── it should revert
 ├── when called
 │   ├── it calls approve on every bound token
 │   └── it calls logBCoWPool on the factory