Merge branch 'dev' of github.com:defi-wonderland/prophet-modules into…

… fix/bytes32-check

docs/src/content/extensions/accounting.md

@@ -18,7 +18,7 @@ The Accounting Extension is a contract that allows users to deposit and bond fun
 
 ## 3. Key Mechanisms & Concepts
 
-- **Deposits**: Users can deposit tokens into the Accounting Extension. These deposits are tracked in a mapping that associates each user's address with their balance of each token. Deposits can be made in any ERC20 token, including wrapped Ether (WETH).
+- **Deposits**: Users can deposit tokens into the Accounting Extension. These deposits are tracked in a mapping that associates each user's address with their balance of each token. Deposits can be made in many ERC20 tokens, including wrapped Ether (WETH).
 
 - **Withdrawals**: Users can withdraw their deposited tokens at any time, provided they have sufficient balance. The withdrawal operation reduces the user's balance in the Accounting Extension and transfers the tokens back to the user's address. Locked tokens can't be withdrawn until they're released by a module.
 
@@ -31,5 +31,5 @@ The Accounting Extension is a contract that allows users to deposit and bond fun
 ## 4. Gotchas
 
 - Before depositing ERC20 tokens, users must first approve the Accounting Extension to transfer the tokens on their behalf.
--  Users can only withdraw tokens that are not currently bonded. If a user has bonded tokens for a request, those tokens are locked until they are released by an allowed module. Attempting to withdraw bonded tokens will result in an error. Attempting to slash or pay out tokens that are not locked will also result in a transaction being reverted.
-- The contract supports any ERC20 token, including wrapped Ether (WETH). However, if a user tries to deposit a non-ERC20 token or a token that the contract otherwise doesn't support, the transaction will fail.
+- Users can only withdraw tokens that are not currently bonded. If a user has bonded tokens for a request, those tokens are locked until they are released by an allowed module. Attempting to withdraw bonded tokens will result in an error. Attempting to slash or pay out tokens that are not locked will also result in a transaction being reverted.
+- The contract supports many ERC20 tokens, including wrapped Ether (WETH). However, if a user tries to deposit a ERC20 token with a fee on transfer, a non-ERC20 token or a token that the contract otherwise doesn't support, the transaction will fail.

docs/src/content/extensions/bond_escalation_accounting.md

@@ -21,10 +21,12 @@ The `BondEscalationAccounting` contract is an extension that allows users to dep
 
 - Pledging: Users can pledge tokens for or against a dispute. The pledged tokens are locked and cannot be used until the dispute is resolved.
 
-- Deposits: Users can deposit tokens into the extension. Deposits can be made in any ERC20 token, ETH deposits will be converted to WETH.
+- Deposits: Users can deposit tokens into the extension. Deposits can be made in many ERC20 tokens, ETH deposits will be converted to WETH.
 
 - Withdrawals: Users can withdraw their tokens at any time. The withdrawal operation reduces the user's balance in the extension and transfers the tokens back to the user's address. Locked tokens can't be withdrawn until they're released by a module.
 
 ## 4. Gotchas
 
 - Before depositing ERC20 tokens, users must first approve the extension to transfer the tokens on their behalf.
+- Users can only withdraw tokens that are not currently bonded. If a user has bonded tokens for a request, those tokens are locked until they are released by an allowed module. Attempting to withdraw bonded tokens will result in an error. Attempting to slash or pay out tokens that are not locked will also result in a transaction being reverted.
+- The contract supports many ERC20 tokens, including wrapped Ether (WETH). However, if a user tries to deposit a ERC20 token with a fee on transfer, a non-ERC20 token or a token that the contract otherwise doesn't support, the transaction will fail.

solidity/contracts/extensions/AccountingExtension.sol

@@ -52,8 +52,14 @@ contract AccountingExtension is IAccountingExtension {
 
   /// @inheritdoc IAccountingExtension
   function deposit(IERC20 _token, uint256 _amount) external {
+    uint256 _balance = _token.balanceOf(address(this));
+
     _token.safeTransferFrom(msg.sender, address(this), _amount);
+
+    if (_amount != _token.balanceOf(address(this)) - _balance) revert AccountingExtension_FeeOnTransferToken();
+
     balanceOf[msg.sender][_token] += _amount;
+
     emit Deposited(msg.sender, _token, _amount);
   }
 

solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol

@@ -126,14 +126,12 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
     if (block.timestamp < _escalation.startTime + _params.committingTimeWindow) {
       revert PrivateERC20ResolutionModule_OnGoingCommittingPhase();
     }
-    if (block.timestamp < _escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow) {
+    if (block.timestamp <= _escalation.startTime + _params.committingTimeWindow + _params.revealingTimeWindow) {
       revert PrivateERC20ResolutionModule_OnGoingRevealingPhase();
     }
 
     uint256 _quorumReached = _escalation.totalVotes >= _params.minVotesForQuorum ? 1 : 0;
 
-    address[] memory __voters = _voters[_disputeId].values();
-
     if (_quorumReached == 1) {
       ORACLE.updateDisputeStatus(_request, _response, _dispute, IOracle.DisputeStatus.Won);
       emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Won);
@@ -142,9 +140,11 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
       emit DisputeResolved(_dispute.requestId, _disputeId, IOracle.DisputeStatus.Lost);
     }
 
-    uint256 _length = __voters.length;
-    for (uint256 _i; _i < _length;) {
-      _params.votingToken.safeTransfer(__voters[_i], _votersData[_disputeId][__voters[_i]].numOfVotes);
+    address _voter;
+    uint256 _votersLength = _voters[_disputeId].length();
+    for (uint256 _i; _i < _votersLength;) {
+      _voter = _voters[_disputeId].at(_i);
+      _params.votingToken.safeTransfer(_voter, _votersData[_disputeId][_voter].numOfVotes);
       unchecked {
         ++_i;
       }

solidity/interfaces/extensions/IAccountingExtension.sol

@@ -64,6 +64,11 @@ interface IAccountingExtension {
                               ERRORS
   //////////////////////////////////////////////////////////////*/
 
+  /**
+   * @notice Thrown when depositing tokens with a fee on transfer
+   */
+  error AccountingExtension_FeeOnTransferToken();
+
   /**
    * @notice Thrown when the account doesn't have enough balance to bond/withdraw
    * or not enough bonded to release/pay
@@ -121,6 +126,7 @@ interface IAccountingExtension {
   /**
    * @notice Transfers tokens from a user and updates his virtual balance
    * @dev The user must have approved the accounting extension to transfer the tokens.
+   * The transfer must not take a fee (deflationary tokens can lead to unexpected behavior).
    * @param _token The address of the token being deposited
    * @param _amount The amount of `_token` to deposit
    */

solidity/test/mocks/MockERC20Proxy.sol

@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.19;
+
+import {IERC20} from '@openzeppelin/contracts/token/ERC20/IERC20.sol';
+
+contract MockERC20Proxy {
+  IERC20 public token;
+
+  mapping(uint256 _callCount => mapping(address _account => uint256 _amount)) internal _balancesPerCall;
+  uint256 internal _calls;
+  bool internal _mocked;
+
+  constructor(IERC20 _token) {
+    token = _token;
+  }
+
+  function mockBalanceOfPerCall(uint256 _callCount, address _account, uint256 _amount) external {
+    _balancesPerCall[_callCount][_account] = _amount;
+    _mocked = true;
+  }
+
+  function balanceOf(address _account) external view returns (uint256 _amount) {
+    if (_mocked) {
+      _amount = _balancesPerCall[_calls][_account];
+    } else {
+      _amount = token.balanceOf(_account);
+    }
+  }
+
+  // solhint-disable-next-line
+  fallback() external {
+    if (_mocked) {
+      ++_calls;
+    }
+    (bool _success,) = address(token).call(msg.data);
+    require(_success, 'MockERC20Proxy: call failed');
+  }
+}

solidity/test/unit/extensions/AccountingExtension.t.sol

@@ -11,6 +11,8 @@ import {
 import {IOracle} from '@defi-wonderland/prophet-core-contracts/solidity/contracts/Oracle.sol';
 import {EnumerableSet} from '@openzeppelin/contracts/utils/structs/EnumerableSet.sol';
 
+import {MockERC20Proxy} from '../../mocks/MockERC20Proxy.sol';
+
 contract ForTest_AccountingExtension is AccountingExtension {
   using EnumerableSet for EnumerableSet.AddressSet;
 
@@ -50,52 +52,89 @@ contract BaseTest is Test, Helpers {
   /**
    * @notice Deploy the target and mock oracle extension
    */
-  function setUp() public {
-    oracle = IOracle(makeAddr('Oracle'));
-    vm.etch(address(oracle), hex'069420');
-
-    token = IERC20(makeAddr('Token'));
-    vm.etch(address(token), hex'069420');
+  function setUp() public virtual {
+    oracle = IOracle(_mockContract('Oracle'));
+    token = IERC20(_mockContract('Token'));
 
     extension = new ForTest_AccountingExtension(oracle);
   }
 }
 
 contract AccountingExtension_Unit_DepositAndWithdraw is BaseTest {
+  MockERC20Proxy public tokenProxy;
+
+  /**
+   * @notice Deploy a token proxy
+   */
+  function setUp() public override {
+    super.setUp();
+
+    tokenProxy = new MockERC20Proxy(token);
+  }
+
   /**
    * @notice Test an ERC20 deposit
    */
-  function test_depositERC20(uint256 _amount) public {
+  function test_depositERC20(uint256 _amount, uint256 _initialBalance) public {
+    // Mock and expect the ERC20 balance
+    _initialBalance = bound(_initialBalance, 0, type(uint256).max - _amount);
+    tokenProxy.mockBalanceOfPerCall(0, address(extension), _initialBalance);
+    tokenProxy.mockBalanceOfPerCall(1, address(extension), _initialBalance + _amount);
+    vm.expectCall(address(tokenProxy), abi.encodeCall(IERC20.balanceOf, (address(extension))), 2);
+
     // Mock and expect the ERC20 transfer
     _mockAndExpect(
       address(token), abi.encodeCall(IERC20.transferFrom, (sender, address(extension), _amount)), abi.encode(true)
     );
 
     // Expect the event
-    vm.expectEmit(true, true, true, true, address(extension));
-    emit Deposited(sender, token, _amount);
+    _expectEmit(address(extension));
+    emit Deposited(sender, IERC20(address(tokenProxy)), _amount);
 
     vm.prank(sender);
-    extension.deposit(token, _amount);
+    extension.deposit(IERC20(address(tokenProxy)), _amount);
 
     // Check: balance of token deposit increased?
-    assertEq(extension.balanceOf(sender, token), _amount);
+    assertEq(extension.balanceOf(sender, IERC20(address(tokenProxy))), _amount);
+  }
+
+  /**
+   * @notice Should revert if token takes a fee on transfer
+   */
+  function test_depositRevert(uint256 _amount, uint256 _fee, uint256 _initialBalance) public {
+    vm.assume(_amount >= _fee);
+    vm.assume(_fee > 0);
+
+    // Mock and expect the ERC20 balance
+    _initialBalance = bound(_initialBalance, 0, type(uint256).max - (_amount - _fee));
+    tokenProxy.mockBalanceOfPerCall(0, address(extension), _initialBalance);
+    tokenProxy.mockBalanceOfPerCall(1, address(extension), _initialBalance + (_amount - _fee));
+    vm.expectCall(address(tokenProxy), abi.encodeCall(IERC20.balanceOf, (address(extension))), 2);
+
+    // Mock and expect the ERC20 transfer
+    _mockAndExpect(
+      address(token), abi.encodeCall(IERC20.transferFrom, (sender, address(extension), _amount)), abi.encode(true)
+    );
+
+    // Check: does it revert if token takes a fee on transfer?
+    vm.expectRevert(IAccountingExtension.AccountingExtension_FeeOnTransferToken.selector);
+    vm.prank(sender);
+    extension.deposit(IERC20(address(tokenProxy)), _amount);
   }
 
   /**
    * @notice Test withdrawing ERC20. Should update balance and emit event
    */
   function test_withdrawERC20(uint256 _amount, uint256 _initialBalance) public {
-    vm.assume(_amount > 0);
-
     // Set the initial balance
     _initialBalance = bound(_initialBalance, _amount, type(uint256).max);
     extension.forTest_setBalanceOf(sender, token, _initialBalance);
 
+    // Mock and expect the ERC20 transfer
     _mockAndExpect(address(token), abi.encodeCall(IERC20.transfer, (sender, _amount)), abi.encode(true));
 
     // Expect the event
-    vm.expectEmit(true, true, true, true, address(extension));
+    _expectEmit(address(extension));
     emit Withdrew(sender, token, _amount);
 
     vm.prank(sender);
@@ -148,7 +187,7 @@ contract AccountingExtension_Unit_Bond is BaseTest {
     extension.forTest_setBalanceOf(_bonder, token, _initialBalance);
 
     // Check: is the event emitted?
-    vm.expectEmit(true, true, true, true, address(extension));
+    _expectEmit(address(extension));
     emit Bonded(_requestId, _bonder, token, _amount);
 
     vm.prank(_sender);
@@ -283,7 +322,7 @@ contract AccountingExtension_Unit_Pay is BaseTest {
     extension.forTest_setBondedBalanceOf(_requestId, _payer, token, _initialBalance);
 
     // Check: is the event emitted?
-    vm.expectEmit(true, true, true, true, address(extension));
+    _expectEmit(address(extension));
     emit Paid(_requestId, _receiver, _payer, token, _amount);
 
     vm.prank(_sender);
@@ -375,7 +414,7 @@ contract AccountingExtension_Unit_Release is BaseTest {
     extension.forTest_setBondedBalanceOf(_requestId, _bonder, token, _initialBalance);
 
     // Check: is the event emitted?
-    vm.expectEmit(true, true, true, true, address(extension));
+    _expectEmit(address(extension));
     emit Released(_requestId, _bonder, token, _amount);
 
     vm.prank(_sender);

solidity/test/unit/modules/resolution/ArbitratorModule.t.sol

@@ -80,7 +80,7 @@ contract ArbitratorModule_Unit_ModuleData is BaseTest {
    * @notice Test that the status is correctly retrieved
    */
   function test_getStatus(uint256 _status, bytes32 _disputeId) public {
-    _status = bound(_status, 0, uint256(IArbitratorModule.ArbitrationStatus.Resolved));
+    vm.assume(_status <= uint256(IArbitratorModule.ArbitrationStatus.Resolved));
     IArbitratorModule.ArbitrationStatus _arbitratorStatus = IArbitratorModule.ArbitrationStatus(_status);
 
     // Store the mock dispute
@@ -233,8 +233,7 @@ contract ArbitratorModule_Unit_ResolveDispute is BaseTest {
   }
 
   function test_emitsEvent(uint256 _status, address _arbitrator) public assumeFuzzable(_arbitrator) {
-    vm.assume(_status <= uint256(IOracle.DisputeStatus.Lost));
-    vm.assume(_status > uint256(IOracle.DisputeStatus.Escalated));
+    _status = bound(_status, uint256(IOracle.DisputeStatus.Escalated) + 1, uint256(IOracle.DisputeStatus.Lost));
     IOracle.DisputeStatus _arbitratorStatus = IOracle.DisputeStatus(_status);
 
     mockRequest.resolutionModuleData = abi.encode(_arbitrator);