openst / openst-contracts Goto Github PK
View Code? Open in Web Editor NEWOpenST is a framework for building token economies on Ethereum
License: Apache License 2.0
OpenST is a framework for building token economies on Ethereum
License: Apache License 2.0
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:
Below tasks should be covered in this PR:
Different projects (mosaic-contracts, openst-contracts, etc) are presenting the signature differently:
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:
* @param _newWallet Wallt to add.
-- change Wallt to Wallet.copyright 2017
to copyright 2018
// ----------------------------------------------------------------------------
// Utility Chain: MultiSigWallet
//
// http://www.simpletoken.org/
//
// ----------------------------------------------------------------------------
keyIsActive
and keyWasAuthorized
are dead-code modifiers; please remove.keyWasNotAuthorized
should be keyDoesNotExist
or keyIsNotAuthorized
keyHasNotExpired
is dead code and should be removed.submitAuthorizeSession
.keyIsNotNull(_ephemeralKey)
) is redundant as implied by keyIsAuthorized
.processExecutableTransaction
; possible verifyExecutableTransaction
.isEphemeralKeyActive
in verifyExecutableTransaction
(processExecutableTransaction) (no need for explicit view function); better readability and no need to read from storage twice.recoverKey
function and use ecrecover
directly.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:
Below tasks will be taken care
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."
);
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)
.
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()
In the context of the following ticket, please, check also the following functions pre-condition:
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.
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 theexecuteRule
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.
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.
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:
The two sets of interactions:
finalise --> finalize
utilise --> utilize
organise --> organize
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.