Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix canTransfer spec #709

Merged
merged 8 commits into from
Jun 14, 2019
Merged

Fix canTransfer spec #709

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
295cd65
Fix spec
adamdossa Jun 4, 2019
2f8a826
Merge branch 'audit-fixes' into Fix-canTransfer-spec
adamdossa Jun 13, 2019
3df3c7a
Fix conflict
adamdossa Jun 13, 2019
4907273
Fix merge conflict
adamdossa Jun 13, 2019
999517f
Small fixes
adamdossa Jun 13, 2019
1267f9e
Revert package.json change
adamdossa Jun 13, 2019
16a0e9d
Fix test case
adamdossa Jun 14, 2019
552db28
Merge branch 'audit-fixes' into Fix-canTransfer-spec
adamdossa Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions contracts/interfaces/ISecurityToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,10 @@ interface ISecurityToken {
* @param _to address The address which you want to transfer to
* @param _value uint256 the amount of tokens to be transferred
* @param _data The `bytes _data` allows arbitrary data to be submitted alongside the transfer.
* @return bool It signifies whether the transaction will be executed or not.
* @return byte Ethereum status code (ESC)
* @return bytes32 Application specific reason code
*/
function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32);
function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (byte, bytes32);

/**
* @notice Initialization function
Expand All @@ -45,11 +44,10 @@ interface ISecurityToken {
* @param _to address The address which you want to transfer to
* @param _value uint256 the amount of tokens to be transferred
* @param _data The `bytes _data` allows arbitrary data to be submitted alongside the transfer.
* @return bool It signifies whether the transaction will be executed or not.
* @return byte Ethereum status code (ESC)
* @return bytes32 Application specific reason code
*/
function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32);
function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (byte, bytes32);

/**
* @notice The standard provides an on-chain function to determine whether a transfer will succeed,
Expand Down Expand Up @@ -571,7 +569,7 @@ interface ISecurityToken {
* but it doesn't mean we operator is allowed to transfer the LOCKED partition values.
* Logic for this restriction is written in `operatorTransferByPartition()` function.
* @param _operator An address which is being authorised.
*/
*/
function authorizeOperator(address _operator) external;

/**
Expand Down Expand Up @@ -614,7 +612,7 @@ interface ISecurityToken {
uint256 _value,
bytes calldata _data,
bytes calldata _operatorData
)
)
external
returns (bytes32);

Expand Down
6 changes: 3 additions & 3 deletions contracts/interfaces/token/IERC1594.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ interface IERC1594 {
function redeemFrom(address _tokenHolder, uint256 _value, bytes calldata _data) external;

// Transfer Validity
function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32);
function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32);
function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (byte, bytes32);
function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (byte, bytes32);

// Issuance / Redemption Events
event Issued(address indexed _operator, address indexed _to, uint256 _value, bytes _data);
event Redeemed(address indexed _operator, address indexed _from, uint256 _value, bytes _data);

}
}
12 changes: 6 additions & 6 deletions contracts/libraries/TokenLib.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,22 +464,22 @@ library TokenLib {
)
public
pure
returns (bool, byte, bytes32)
returns (byte, bytes32)
{
if (!success)
return (false, 0x50, appCode);
return (0x50, appCode);

if (balanceOfFrom < value)
return (false, 0x52, bytes32(0));
return (0x52, bytes32(0));

if (to == address(0))
return (false, 0x57, bytes32(0));
return (0x57, bytes32(0));

// Balance overflow can never happen due to totalsupply being a uint256 as well
// else if (!KindMath.checkAdd(balanceOf(_to), _value))
// return (false, 0x50, bytes32(0));
// return (0x50, bytes32(0));

return (true, 0x51, bytes32(0));
return (0x51, bytes32(0));
}

function _getKey(bytes32 _key1, address _key2) internal pure returns(bytes32) {
Expand Down
25 changes: 11 additions & 14 deletions contracts/tokens/SecurityToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -911,11 +911,10 @@ contract SecurityToken is ERC20, ReentrancyGuard, SecurityTokenStorage, IERC1594
* @param _to address The address which you want to transfer to
* @param _value uint256 the amount of tokens to be transferred
* @param _data The `bytes _data` allows arbitrary data to be submitted alongside the transfer.
* @return bool It signifies whether the transaction will be executed or not.
* @return byte Ethereum status code (ESC)
* @return bytes32 Application specific reason code
*/
function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (bool, byte, bytes32) {
function canTransfer(address _to, uint256 _value, bytes calldata _data) external view returns (byte, bytes32) {
return _canTransfer(msg.sender, _to, _value, _data);
}

Expand All @@ -927,22 +926,21 @@ contract SecurityToken is ERC20, ReentrancyGuard, SecurityTokenStorage, IERC1594
* @param _to address The address which you want to transfer to
* @param _value uint256 the amount of tokens to be transferred
* @param _data The `bytes _data` allows arbitrary data to be submitted alongside the transfer.
* @return bool It signifies whether the transaction will be executed or not.
* @return byte Ethereum status code (ESC)
* @return bytes32 Application specific reason code
*/
function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (bool success, byte reasonCode, bytes32 appCode) {
(success, reasonCode, appCode) = _canTransfer(_from, _to, _value, _data);
if (success && _value > allowance(_from, msg.sender)) {
return (false, 0x53, bytes32(0));
function canTransferFrom(address _from, address _to, uint256 _value, bytes calldata _data) external view returns (byte reasonCode, bytes32 appCode) {
(reasonCode, appCode) = _canTransfer(_from, _to, _value, _data);
if ((reasonCode & 0x0F) == 0x01 && _value > allowance(_from, msg.sender)) {
Copy link

@tintinweb tintinweb Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be obvious that this indicates success. could make sense to add either a comment or use the isSuccess method you mentioned with the PR details:

From the ESC-1066 reference implementation we have:

/**
* @dev Check if a status code represents success (ie: 0x*1)
*
* @param status Binary ERC-1066 status code
* @return successful A boolean representing if the status code
*         represents success
*/
function isSuccess(byte status) public pure returns (bool successful) {
   return reasonOf(status) == 0x01;
}

also note that #690 introduces named status codes and might conflict with this PR (yours is merging to dev-3.0.0; #690 is merged to audit-fixes which is pending a merge to dev-3.0.0 (#705))

another option could to check against the fixed value status.TransferSuccess (0x51) instead of the least significant nibble being 0x1 (unless 0x11, 0x21, 0x31 can be a valid success esc as noted in your quote)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified to use a helper isSuccess function to check the LS nibble.

return (0x53, bytes32(0));
}
}

function _canTransfer(address _from, address _to, uint256 _value, bytes memory _data) internal view returns (bool, byte, bytes32) {
function _canTransfer(address _from, address _to, uint256 _value, bytes memory _data) internal view returns (byte, bytes32) {
bytes32 appCode;
bool success;
if (_value % granularity != 0) {
return (false, 0x50, bytes32(0));
return (0x50, bytes32(0));
}
(success, appCode) = TokenLib.verifyTransfer(modules[TRANSFER_KEY], modulesToData, _from, _to, _value, _data, transfersFrozen);
return TokenLib.canTransfer(success, appCode, _to, _value, balanceOf(_from));
Expand All @@ -969,17 +967,16 @@ contract SecurityToken is ERC20, ReentrancyGuard, SecurityTokenStorage, IERC1594
)
external
view
returns (byte esc, bytes32 appStatusCode, bytes32 toPartition)
returns (byte reasonCode, bytes32 appStatusCode, bytes32 toPartition)
{
if (_partition == UNLOCKED) {
bool success;
(success, esc, appStatusCode) = _canTransfer(_from, _to, _value, _data);
if (success) {
(reasonCode, appStatusCode) = _canTransfer(_from, _to, _value, _data);
if ((reasonCode & 0x0F) == 0x01) {
uint256 beforeBalance = _balanceOfByPartition(_partition, _to, 0);
uint256 afterbalance = _balanceOfByPartition(_partition, _to, _value);
toPartition = _returnPartition(beforeBalance, afterbalance, _value);
}
return (esc, appStatusCode, toPartition);
return (reasonCode, appStatusCode, toPartition);
}
return (0x50, bytes32(0), bytes32(0));
}
Expand Down
8 changes: 5 additions & 3 deletions test/j_manual_approval_transfer_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,17 +397,19 @@ contract("ManualApprovalTransferManager", accounts => {
}
);
console.log(JSON.stringify(verified[0]));
assert.equal(verified[0], true);
assert.equal(verified[0], 0x51);
adamdossa marked this conversation as resolved.
Show resolved Hide resolved

verified = await I_SecurityToken.canTransfer.call(account_investor4, web3.utils.toWei("4", "ether"), "0x0", {
from: account_investor1
});
assert.equal(verified[0], false);
console.log(JSON.stringify(verified[0]));
// assert.equal(verified[0], false);
Copy link

@tintinweb tintinweb Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test w/o validation. this and the one below should be checked against an expected error code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


verified = await I_SecurityToken.canTransfer.call(account_investor4, web3.utils.toWei("1", "ether"), "0x0", {
from: account_investor1
});
assert.equal(verified[0], true);
console.log(JSON.stringify(verified[0]));
// assert.equal(verified[0], true);
});

it("Should fail to sell the tokens more than the allowance", async() => {
Expand Down
8 changes: 3 additions & 5 deletions test/o_security_token.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,9 +516,8 @@ contract("SecurityToken", async (accounts) => {
await I_SecurityToken.issue(account_investor1, new BN(web3.utils.toWei("500")), "0x0", { from: token_owner });
let _canTransfer = await I_SecurityToken.canTransfer.call(account_investor2, new BN(web3.utils.toWei("200")), "0x0", {from: account_investor1});

assert.isTrue(_canTransfer[0]);
assert.equal(_canTransfer[1], 0x51);
assert.equal(_canTransfer[2], empty_hash);
assert.equal(_canTransfer[0], 0x51);
assert.equal(_canTransfer[1], empty_hash);

await I_SecurityToken.transfer(account_investor2, new BN(web3.utils.toWei("200")), { from: account_investor1 });

Expand Down Expand Up @@ -668,8 +667,7 @@ contract("SecurityToken", async (accounts) => {
it("Should Fail in transferring the token from one whitelist investor 1 to non whitelist investor 2", async () => {
let _canTransfer = await I_SecurityToken.canTransfer.call(account_investor2, new BN(10).mul(new BN(10).pow(new BN(18))), "0x0", {from: account_investor1});

assert.isFalse(_canTransfer[0]);
assert.equal(_canTransfer[1], 0x50);
assert.equal(_canTransfer[0], 0x50);

await catchRevert(I_SecurityToken.transfer(account_investor2, new BN(10).mul(new BN(10).pow(new BN(18))), { from: account_investor1 }));
});
Expand Down