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

Fixed CTM verifyTransfer bug #736

Merged
merged 6 commits into from
Jul 8, 2019
Merged

Fixed CTM verifyTransfer bug #736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0d541c1
Fixed CTM canTransferBug
maxsam4 Jul 3, 2019
994bdd5
Added test case
maxsam4 Jul 3, 2019
4753504
Merge branch 'dev-3.0.0' into ctm-canTransfer-fix
adamdossa Jul 3, 2019
9986f98
Merge branch 'dev-3.0.0' into ctm-canTransfer-fix
satyamakgec Jul 4, 2019
fdd421d
Merge branch 'dev-3.0.0' into ctm-canTransfer-fix
adamdossa Jul 5, 2019
5a5be38
Added comment for devs
maxsam4 Jul 8, 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
25 changes: 21 additions & 4 deletions contracts/modules/TransferManager/CTM/CountTransferManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,16 @@ contract CountTransferManager is CountTransferManagerStorage, TransferManager {
external
returns(Result)
{
(Result success, ) = _verifyTransfer(_from, _to, _amount);
(Result success, ) = _verifyTransfer(_from, _to, _amount, securityToken.holderCount());
return success;
}

/**
* @notice Used to verify the transfer transaction and prevent a transfer if it passes the allowed amount of token holders
* @dev module.verifyTransfer is called by SecToken.canTransfer and does not receive the updated holderCount therefore
* verifyTransfer has to manually account for pot. tokenholder changes (by mimicking TokenLib.adjustInvestorCount).
* module.executeTransfer is called by SecToken.transfer|issue|others and receives an updated holderCount
* as sectoken calls TokenLib.adjustInvestorCount before executeTransfer.
* @param _from Address of the sender
* @param _to Address of the receiver
* @param _amount Amount to send
Expand All @@ -53,20 +57,33 @@ contract CountTransferManager is CountTransferManagerStorage, TransferManager {
view
returns(Result, bytes32)
{
return _verifyTransfer(_from, _to, _amount);
uint256 holderCount = securityToken.holderCount();
if (_amount != 0 && _from != _to) {
// Check whether receiver is a new token holder
if (_to != address(0) && securityToken.balanceOf(_to) == 0) {
holderCount++;
}
// Check whether sender is moving all of their tokens
if (_amount == securityToken.balanceOf(_from)) {
holderCount--;
Copy link

Choose a reason for hiding this comment

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

guard holderCount from over/underflows as it can be zero. See existing implementation in TokenLib.adjustInvestorCount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tintinweb Since holderCount can only be incremented/decremented by one at a time, it can not be overflowed.

holderCount cannot be underflowed either since holderCount must be greater than 1 if _amount != 0 && _from != _to (checked on L57).

Earlier, we used SafeMath everywhere (including TokenLib.adjustInvestorCount) but lately, I have using SafeMath only where it is needed (for optimization).

Do you have a scenario in mind where holderCount can be underflowed or overflowed?

Copy link

Choose a reason for hiding this comment

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

thanks @maxsam4, agree the upper bound is unrealistic (especially when using ctm :)) and holderCount==0 with amount !=0 is mutually exclusive 👍

}
}

return _verifyTransfer(_from, _to, _amount, holderCount);
}

function _verifyTransfer(
address _from,
address _to,
uint256 _amount
uint256 _amount,
uint256 _holderCount
)
internal
view
returns(Result, bytes32)
{
if (!paused) {
if (maxHolderCount < securityToken.holderCount()) {
if (maxHolderCount < _holderCount) {
// Allow transfers to existing maxHolders
if (securityToken.balanceOf(_to) != 0 || securityToken.balanceOf(_from) == _amount) {
return (Result.NA, bytes32(0));
Expand Down
3 changes: 3 additions & 0 deletions test/d_count_transfer_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,9 @@ contract("CountTransferManager", async (accounts) => {
);

await catchRevert(I_SecurityToken.issue(account_investor3, new BN(web3.utils.toWei("3", "ether")), "0x0", { from: token_owner }));
await catchRevert(I_SecurityToken.transfer(account_investor3, new BN(web3.utils.toWei("1", "ether")), { from: account_investor2 }));
let canTransfer = await I_SecurityToken.canTransfer(account_investor3, new BN(web3.utils.toWei("1", "ether")), "0x0", { from: account_investor2 });
assert.equal(canTransfer[0], "0x50"); //Transfer failure.
});

it("Should still be able to add to original token holders", async () => {
Expand Down