[Internal audit] aragonOS 5 (#612)

* IArbitrable: align ERC165 implementation with other contracts

* AragonApp: simplify ERC165 interface tests

* DisputableAragonApp: polish tests

* docs: polish inline comments

* cosmetic: add review notes

* arbitration: add app fees cashier inline doc

* arbitration: mark app fees cashier `payAppFees` as payable

* agreement: do not force IArbitrable and ACLOracle support

* disputable: rename `_newAgreementAction` to `_registerDisputableAction`

* disputable: rename `_closeAgreementAction` to `_closeDisputableAction`

* agreement: implement default return values for mock

* test: remove note

* agreement: trim interface removing arbitration contracts

* agreement: remove activation related functions

Co-authored-by: Facu Spagnuolo <facundo_spagnuolo@icloud.com>

contracts/apps/disputable/DisputableAragonApp.sol

@@ -7,8 +7,8 @@ pragma solidity ^0.4.24;
 import "./IAgreement.sol";
 import "./IDisputable.sol";
 import "../AragonApp.sol";
-import "../../lib/token/ERC20.sol";
 import "../../lib/math/SafeMath64.sol";
+import "../../lib/token/ERC20.sol";
 
 
 contract DisputableAragonApp is IDisputable, AragonApp {
@@ -35,7 +35,7 @@ contract DisputableAragonApp is IDisputable, AragonApp {
     /**
     * @notice Challenge disputable action #`_disputableActionId`
     * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier
-    *      is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook.
+    *      is included. Subclasses should implement the internal implementation of the hook.
     * @param _disputableActionId Identifier of the action to be challenged
     * @param _challengeId Identifier of the challenge in the context of the Agreement
     * @param _challenger Address that submitted the challenge
@@ -47,7 +47,7 @@ contract DisputableAragonApp is IDisputable, AragonApp {
     /**
     * @notice Allow disputable action #`_disputableActionId`
     * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier
-    *      is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook.
+    *      is included. Subclasses should implement the internal implementation of the hook.
     * @param _disputableActionId Identifier of the action to be allowed
     */
     function onDisputableActionAllowed(uint256 _disputableActionId) external onlyAgreement {
@@ -57,7 +57,7 @@ contract DisputableAragonApp is IDisputable, AragonApp {
     /**
     * @notice Reject disputable action #`_disputableActionId`
     * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier
-    *      is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook.
+    *      is included. Subclasses should implement the internal implementation of the hook.
     * @param _disputableActionId Identifier of the action to be rejected
     */
     function onDisputableActionRejected(uint256 _disputableActionId) external onlyAgreement {
@@ -67,7 +67,7 @@ contract DisputableAragonApp is IDisputable, AragonApp {
     /**
     * @notice Void disputable action #`_disputableActionId`
     * @dev This hook must be implemented by Disputable apps. We provide a base implementation to ensure that the `onlyAgreement` modifier
-    *      is included. Developers of derived Disputable apps should implement an internal abstract implementation of the hook.
+    *      is included. Subclasses should implement the internal implementation of the hook.
     * @param _disputableActionId Identifier of the action to be voided
     */
     function onDisputableActionVoided(uint256 _disputableActionId) external onlyAgreement {
@@ -130,22 +130,22 @@ contract DisputableAragonApp is IDisputable, AragonApp {
     function _onDisputableActionVoided(uint256 _disputableActionId) internal;
 
     /**
-    * @dev Create a new action in the Agreement
+    * @dev Register a new disputable action in the Agreement
     * @param _disputableActionId Identifier of the action in the context of the Disputable
     * @param _context Link to human-readable context for the given action
     * @param _submitter Address that submitted the action
     * @return Unique identifier for the created action in the context of the Agreement
     */
-    function _newAgreementAction(uint256 _disputableActionId, bytes _context, address _submitter) internal returns (uint256) {
+    function _registerDisputableAction(uint256 _disputableActionId, bytes _context, address _submitter) internal returns (uint256) {
         IAgreement agreement = _ensureAgreement();
         return agreement.newAction(_disputableActionId, _context, _submitter);
     }
 
     /**
-    * @dev Close action in the Agreement
+    * @dev Close disputable action in the Agreement
     * @param _actionId Identifier of the action in the context of the Agreement
     */
-    function _closeAgreementAction(uint256 _actionId) internal {
+    function _closeDisputableAction(uint256 _actionId) internal {
         IAgreement agreement = _ensureAgreement();
         agreement.closeAction(_actionId);
     }

contracts/apps/disputable/IAgreement.sol

@@ -4,19 +4,11 @@
 
 pragma solidity ^0.4.24;
 
-import "../../acl/IACLOracle.sol";
 import "../../lib/token/ERC20.sol";
-import "../../lib/arbitration/IArbitrable.sol";
-import "../../lib/arbitration/IAragonAppFeesCashier.sol";
 
 
-contract IAgreement is IArbitrable, IACLOracle {
+contract IAgreement {
 
-    event Signed(address indexed signer, uint256 settingId);
-    event SettingChanged(uint256 settingId);
-    event DisputableAppActivated(address indexed disputable);
-    event DisputableAppDeactivated(address indexed disputable);
-    event CollateralRequirementChanged(address indexed disputable, uint256 collateralRequirementId);
     event ActionSubmitted(uint256 indexed actionId, address indexed disputable);
     event ActionClosed(uint256 indexed actionId);
     event ActionChallenged(uint256 indexed actionId, uint256 indexed challengeId);
@@ -35,19 +27,6 @@ contract IAgreement is IArbitrable, IACLOracle {
         Voided
     }
 
-    function sign() external;
-
-    function activate(
-        address _disputable,
-        ERC20 _collateralToken,
-        uint256 _actionAmount,
-        uint256 _challengeAmount,
-        uint64 _challengeDuration
-    )
-        external;
-
-    function deactivate(address _disputable) external;
-
     function newAction(uint256 _disputableActionId, bytes _context, address _submitter) external returns (uint256);
 
     function closeAction(uint256 _actionId) external;
@@ -57,60 +36,4 @@ contract IAgreement is IArbitrable, IACLOracle {
     function settleAction(uint256 _actionId) external;
 
     function disputeAction(uint256 _actionId, bool _finishedSubmittingEvidence) external;
-
-    function getSigner(address _signer) external view returns (uint256 lastSettingIdSigned, bool mustSign);
-
-    function getCurrentSettingId() external view returns (uint256);
-
-    function getSetting(uint256 _settingId) external view
-        returns (
-            IArbitrator arbitrator,
-            IAragonAppFeesCashier aragonAppFeesCashier,
-            string title,
-            bytes content
-        );
-
-    function getDisputableInfo(address _disputable) external view returns (bool activated, uint256 currentCollateralRequirementId);
-
-    function getCollateralRequirement(address _disputable, uint256 _collateralId) external view
-        returns (
-            ERC20 collateralToken,
-            uint256 actionAmount,
-            uint256 challengeAmount,
-            uint64 challengeDuration
-        );
-
-    function getAction(uint256 _actionId) external view
-        returns (
-            address disputable,
-            uint256 disputableActionId,
-            uint256 collateralRequirementId,
-            uint256 settingId,
-            address submitter,
-            bool closed,
-            bytes context,
-            uint256 currentChallengeId
-        );
-
-    function getChallenge(uint256 _challengeId) external view
-        returns (
-            uint256 actionId,
-            address challenger,
-            uint64 endDate,
-            bytes context,
-            uint256 settlementOffer,
-            ChallengeState state,
-            bool submitterFinishedEvidence,
-            bool challengerFinishedEvidence,
-            uint256 disputeId,
-            uint256 ruling
-        );
-
-    function getChallengeArbitratorFees(uint256 _challengeId) external view
-        returns (
-            uint256 challengerArbitratorFeesAmount,
-            ERC20 challengerArbitratorFeesToken,
-            uint256 submitterArbitratorFeesAmount,
-            ERC20 submitterArbitratorFeesToken
-        );
 }

contracts/apps/disputable/IDisputable.sol

@@ -5,8 +5,8 @@
 pragma solidity ^0.4.24;
 
 import "./IAgreement.sol";
-import "../../lib/token/ERC20.sol";
 import "../../lib/standards/ERC165.sol";
+import "../../lib/token/ERC20.sol";
 
 
 contract IDisputable is ERC165 {

contracts/forwarding/IAbstractForwarder.sol

@@ -21,7 +21,8 @@ contract IAbstractForwarder {
 
     /**
     * @dev Tell whether the proposed forwarding path (an EVM script) from the given sender is allowed.
-    *      The implemented `forward()` method **MUST NOT** revert if `canForward()` returns true for the same parameters.
+    *      However, this is not a strict guarantee of safety: the implemented `forward()` method is
+    *      still allowed to revert even if `canForward()` returns true for the same parameters.
     * @return True if the sender's proposed path is allowed
     */
     function canForward(address sender, bytes evmScript) external view returns (bool);

contracts/forwarding/IForwarderFee.sol

@@ -12,7 +12,7 @@ pragma solidity ^0.4.24;
 interface IForwarderFee {
     /**
     * @dev Provide details about the required fee token and amount
-    * @return Fee token and fee amount. If ETH, expects EtherTokenConstant.
+    * @return Fee token and fee amount. If ETH, returns EtherTokenConstant for the `feeToken`.
     */
     function forwardFee() external view returns (address feeToken, uint256 feeAmount);
 }

contracts/forwarding/IForwarderPayable.sol

@@ -11,7 +11,7 @@ import "./IForwarderFee.sol";
 /**
 * @title Payable forwarder interface
 * @dev This is the basic forwarder interface, that only supports forwarding an EVM script.
-*      Unlike `IForwarder`, this interface allows `forward()` to receive ETH and thereby includes the IForwarderFee interface.
+*      Unlike `IForwarder`, this interface allows `forward()` to receive ETH and therefore includes the IForwarderFee interface.
 *      It is **RECOMMENDED** that only apps requiring a payable `forward()` use this interface.
 */
 contract IForwarderPayable is IAbstractForwarder, IForwarderFee {

contracts/forwarding/IForwarderWithContextPayable.sol

@@ -11,7 +11,7 @@ import "./IForwarderFee.sol";
 /**
 * @title Payable forwarder interface requiring context information
 * @dev This forwarder interface allows for additional context to be attached to the action by the sender.
-*      Unlike `IForwarderWithContext`, this interface allows `forward()` to receive ETH and thereby includes the IForwarderFee interface.
+*      Unlike `IForwarderWithContext`, this interface allows `forward()` to receive ETH and therefore includes the IForwarderFee interface.
 *      It is **RECOMMENDED** that only apps requiring a payable `forward()` use this interface.
 */
 contract IForwarderWithContextPayable is IAbstractForwarder, IForwarderFee {