openst / openst-contracts Goto Github PK

Edited after a chat with @pgev:

ExTx add usability because the facilitator can pay gas (among possibly more reasons).
Please read and evaluate if Gnosis Safe's implementation of multi-sig paths using ExTx can be used for redemption via TokenHolder

Task includes below sub tasks:

• Organization contract properties, modifiers, events and behaviours.
• Organized contract properties, modifiers and behaviours.
• Unit test cases.

Below tasks should be covered in this PR:

• TokenRules is Organized.
• RegisterRule, addGlobalConstraint, removeGlobalConstraint are onlyWorker.
• Test cases update.

Different projects (mosaic-contracts, openst-contracts, etc) are presenting the signature differently:

• tuple (r, s, v)
• bytes

In the scope of this ticket, we should align with other projects and have consistent representation.

The execution becomes:

_to.call.value(msg.value)(_data);

Please, improve MultiSigWallet unit tests according to last review comments wrt TokenHolder, TokenRules contracts.

Add test case for constructor and transferFrom method.

Proposal: The following questions should be answered:

• What the project does?
• Why the project is useful?
• How users can get started with the project?
• Where users can get help with your project?
• Who maintains and contributes to the project?

• In MultisigWallet.sol * @param _newWallet Wallt to add. -- change Wallt to Wallet.
• In MultisigWallet.sol give credit to Gnosis MultiSigWallet implementation.
• In MultisigWallet.sol isTransactionConfirmed should return true if transaction was already executed.
• Update copyright 2017 to copyright 2018
• Abandon the following part in files' headers (explicitly mentioning utility/value chains):

    // ----------------------------------------------------------------------------
    // Utility Chain: MultiSigWallet
    //
    // http://www.simpletoken.org/
    //
    // ----------------------------------------------------------------------------

• In TokenHolder.sol -- keyIsActive and keyWasAuthorized are dead-code modifiers; please remove.
• In TokenHolder.sol -- keyWasNotAuthorized should be keyDoesNotExist or keyIsNotAuthorized
• In TokenHolder.sol -- keyHasNotExpired is dead code and should be removed.
• In TokenHolder.sol -- correct the documentation of submitAuthorizeSession.
• In TokenHolder.sol revokeSession() -- checking the key is not null (keyIsNotNull(_ephemeralKey)) is redundant as implied by keyIsAuthorized.
• In TokenHolder.sol -- preferred; better name for processExecutableTransaction; possible verifyExecutableTransaction.
• In TokenHolder.sol -- inline isEphemeralKeyActive in verifyExecutableTransaction (processExecutableTransaction) (no need for explicit view function); better readability and no need to read from storage twice.
• In TokenHolder.sol -- remove recoverKey function and use ecrecover directly.
• In TokenHolder.sol -- reference EIP-1077 in comment line on getMessageHash.

• We should follow a common pattern of using EIP20Interface or ERC20Interface.

• As per discussion we need to follow EIP20Interface everywhere as it's already used in multiple places. e.g. EIP20Interface.

• Also EIP20Interface is latest standard which has support for 0 transfers.

• Update should be done in code, documentation, file names etc.

Show below actions in Organization Sequence diagram:

• Flow for setting owner
• Flow for setting admin
• Flow for adding/removing workers

Below tasks will be taken care

• Session Management in TokenHolder Contract
• Add TokenHolder Utility Functions called by TokenRules
• Update Branded Token Contract Related with TokenHolder
• Multisig wallet implementation for tokenholder[WIP]
• Device/Owner Management

Below tasks needs to be done under this ticket:

• redeem function in TokenHolder as executableTransaction.

• executableData will be constructed in contract.

• Function signature:
  redeem(
  address _to,
  uint256 _amount,
  address _beneficiary,
  uint256 _gasPrice,
  uint256 _gasLimit,
  uint256 _redeemerNonce,
  bytes32 _hashLock,
  uint256 _nonce,
  uint8 _v,
  bytes32 _r,
  bytes32 _s
  )
  public
  payable
  returns (bytes32 _redeemMessageHash)

• Integration with coGatewayRedeemInterface.

• Integration with UtilityTokenInterface.

The stored nonce (per key) should be the consumed nonce; so change order of adding and checking nonce; and update error message on require.

keyData.nonce = keyData.nonce.add(1);

require(
    _nonce == keyData.nonce,
    "Nonce is not equal to the current nonce."
);

• Remove coGateway from TokenHolder constructor.
• Remove TokenHolder::redeem function.
• Remove redeem callPrefix from TokenHolder constants section.
• Remove GatewayRedeemInterface.sol

• Sequence Diagram.
• Spec description.

• Show Organization + Organized relationship
• Show Organized Integration with UBT and it's actions

ExecuteRule was payable because previously we were thinking of doing redeem with executeRule.
Now as we have explicit redeem function as ExTx, analyze if there is still a reasonable need for executeRule to be payable.

cc @abhayks1

Current Implementation
https://github.com/OpenSTFoundation/mosaic-contracts/blob/develop/contracts/gateway/EIP20CoGateway.sol#L590

As per discussion with Gateway team, facilitator will be removed from redeem function.
Accordingly CoGatewayRedeemInterface.sol and TokenHolder.redeem function signature needs to be updated.

The TokenHolder contract serves mainly as a multisig wallet to give users a strong wallet to manage their tokens. Users are not forced to make use of the multisig features, however some users might choose to do so. While analyzing the different parts of the TokenHolder contract, it was found that the multisig requirements can be bypassed by anybody who can sign a transaction with an authorized ephemeralKey. This is due to how the multisig wallet will execute confirmed transactions and how the rule execution is implemented.

As can be seen in the two code snippets below, the multisig executeTransaction function uses call() to execute the internal multisig functions, while executeRule also uses call() but with a different intention. To execute rules an ephemeralKey has to be authorized by the multisig wallet, however depending on the setup a user or even the organizer might be able to sign data with that ephemeralKey. Essentially the control over the ephemeralKey is equal to the control of the required wallets for the multisig features. This means an attacker could sign a executeRule call with the TokenHolder contract as _to and the _data to call one of the internal multisig functions such as addWallet or changeRequirement.

Affected Files:

openst-contracts/contracts/TokenHolder.sol
openst-contracts/contracts/MultiSigWallet.sol

Affected Code:

function executeRule(address _to,bytes _data,uint256 _nonce,uint8 _v,bytes32 _r, bytes32 _s) public payable returns (bool executionStatus_)
{
    bytes32 messageHash = bytes32(0);
    address ephemeralKey = address(0);
    (messageHash, ephemeralKey) = verifyExecutableTransaction(
EXECUTE_RULE_CALLPREFIX, _to, _data, _nonce, _v, _r, _s );
    // [...]
    // solium-disable-next-line security/no-call-value
    executionStatus_ = _to.call.value(msg.value)(_data);
    // [...]
}

Affected Code:

function executeTransaction(uint256 _transactionID) public onlyWallet
    transactionExists(_transactionID)
    transactionIsConfirmedBy(_transactionID, msg.sender)
    transactionIsNotExecuted(_transactionID)
{
    if (isTransactionConfirmed(_transactionID)) {
        Transaction storage t = transactions[_transactionID];
        // solium-disable-next-line security/no-low-level-calls
        if (t.destination.call(t.data)) {
            t.executed = true;
            emit TransactionExecutionSucceeded(_transactionID);
        } else {
            t.executed = false;
            emit TransactionExecutionFailed(_transactionID);
        }
    }
}

PoC:
The following proof of concept code signs the call to the internal multisig function changeRequirement(1). The from and to address is set to the TokenHolder contract. The resulting signed data can be used with executeRule to change the multisig wallet requirement, without having to go through the multisig confirmation steps.

console.log('** Sign the data as per EIP 1077.');
let eip1077SignedData = ephemeralKeyAccount.signEIP1077Transaction({
  from: '0x9240ddc345d7084cc775eb65f91f7194dbbb48d8',
  to: '0x9240ddc345d7084cc775eb65f91f7194dbbb48d8',
  value: 0,
  gasPrice: 0,
  gas: 0,
  data: '0xba51a6df0000000000000000000000000000000000000000000000000000000000000001',
  nonce: 1,
  callPrefix: '0x59793b00'
});

As can be seen from the changeRequirement example above, this could be used to add, remove or replace wallets in the list, thus fully takeover the TokenHolder multisig wallet.
A simple fix would simply prevent executeRule from calling its own contract, which can be achieved by adding a check in executeRule that _to must not be address(this).

• Delete BrandedToken.sol.
• Delete Internal.sol.
• Delete openst-protocol directory. Move the followings to contracts/ directory: mock version of EIP20Token and EIP20Interface.
• Integrate EIP20TokenInterface.sol with TokenHolder and TokenRules
• Remove Workers, OpsManaged.sol, Owned.sol

• Setup solidity coverage toolset.
• Successfully run test coverage for openst-contracts and attach report to ticket.

The options to evaluate:
- https://www.npmjs.com/package/@0x/sol-coverage
- https://www.npmjs.com/package/solidity-coverage

We need to check the following requirements in TokenHolder::authorizeSession()

• keyIsNotAuthorized
• checking expirationHeight

In the context of the following ticket, please, check also the following functions pre-condition:

• TokenHolder::revokeSession
• MultisigWallet::addWallet
• MultisigWallet::removeWallet
• MultisigWallet::replaceWallet
• MultisigWallet::changeRequirement

Currently, submitRevokeSession() and revokeSession() functions are checking onlyMultisig.
Proposal is to loose the check to onlyWallet to ease for user to revoke the key if compromised.

• User should be able to revert redeem operation through an ephemeral key.
• Ekey Nonce should be consumed to stop replay attacks.
• Integerate with coGateway.revertRedemption.

From the assesment:
TokenHolder: Nonce Reused on Failure (Info)*
The proposal EIP 1077 covering "Executable Signed Messages refunded by the contract" recommends that nonces should never be reused:

If the executed transaction fails internally, nonces should still be updated and gas needs to be paid.
As transactions could always fail due to being out of gas, it is recommended to add another function that increments the nonce for a key independently of the executeRule function. This function would be much cheaper and can be used by an application that observed a failed transaction to increment the nonce. To prevent abuse, the function should also require a signed nonce update message.

If SafeMath.sol was updated by Zeppelin, the repo version should be updated appropriately with the latest one.

Additionally, please analyze the differences between the older and updated SafeMath.sol contracts so that we understand the benefits of the update.

• Remove old contracts
• Remove old javascript files
• Cleanup .md files
• Add SOLIDITY_STYLE_GUIDE.md

Reference: https://docs.google.com/document/d/1r3nP6iCf-ks94QQ8PhgfToLDUgvipC6__33waICJc4U/edit

In order to understand if the business ask of assigning expiration times to an airdrop allocation is reasonable,

Please analyze the additional gas cost incurred for executing a transaction when the requirement is :

If there are more than 1 airdrops allocated with an internal actor, the airdrops expiring first must be consumed first. E.g:
TokenHolder Th has been allocated with 3 airdrops A, B & C
Airdrop A expires at block height B(h)
Airdrop B never expires
Airdrop C expires at block height B(h + n)

vs when airdrops do not expire.

cc @rachinkapoor @siscolo128

The proposed signature:

event RuleExecuted(
    address indexed _to,
    bytes4 _functionSelector,
    address _ephemeralKey,
    uint256 _nonce,
    bytes32 _messageHash,
    bool _status
);

The following two cases should be handled:

• Checking calling EIP20 token approve() function.
• Checking calling TokenRules.allowTransfers and TokenRules.disallowTransfers.

The two sets of interactions:

1. Authorizing an ephemeral key via multi-sig
2. Executing a transaction

finalise --> finalize
utilise --> utilize
organise --> organize