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.

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

Fix the returnPartition function #680

Merged
merged 3 commits into from Jun 12, 2019
Merged

Fix the returnPartition function #680

merged 3 commits into from Jun 12, 2019

Conversation

satyamakgec
Copy link
Contributor

No description provided.

@@ -527,11 +527,11 @@ contract SecurityToken is ERC20, ReentrancyGuard, SecurityTokenStorage, IERC1594
// require(balanceOfByPartition(_partition, msg.sender) >= _value);
// NB - Above condition will be automatically checked using the executeTransfer() function execution.
// NB - passing `_additionalBalance` value is 0 because accessing the balance before transfer
uint256 balanceBeforeTransferLocked = _balanceOfByPartition(_partition, _to, 0);
uint256 lockedBalanceBeforeTransfer = _balanceOfByPartition(LOCKED, _to, 0);

Choose a reason for hiding this comment

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

argument _partition is required to be UNLOCKED in _isValidPartition but it is not used in _balanceOfByPartition which instead hardcodes LOCKED. toPartition will always be UNLOCKED because of a bug in _returnPartition. finally an event gets emitted with the unused _partition (which needs to be UNLOCKED from the function call args.

I am struggling to understand the changes. @satyamakgec I would appreciate if you could elaborate on how this is working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the Idea is that tokenHolder only allows transacting the UNLOCKED token. _isValidPartition() is used to validate whether the partition is UNLOCKED or not. Then function checks the earlier LOCKED balance of the _to and after transfer, it will check the new LOCKED balance of _to so _returnPartition checks if the difference between both locked balance is equal to the value that is being transferred then it means all value is transferred to LOCKED partition. If not then it means some(or full) value transfferd to UNLOCKED partition of the receiver(_to).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I agree on the 542 line change I should add the else statement.

@tintinweb
Copy link

Hi @satyamakgec,

  1. _returnPartition: if line 542 sets toPartition to LOCKED it is always overridden at line 544 to be UNLOCKED. This is still the case with this PR.
    function _returnPartition(uint256 _beforeBalance, uint256 _afterBalance, uint256 _value) internal pure returns(bytes32 toPartition) {
    // return LOCKED only when the transaction `_value` should be equal to the change in the LOCKED partition
    // balance otherwise return UNLOCKED
    if (_afterBalance.sub(_beforeBalance) == _value)
    toPartition = LOCKED;
    // Returning the same partition UNLOCKED
    toPartition = UNLOCKED;

From the issue reported in the initial report:

It's commented in the function that it should return LOCKED under some circumstances.
Function _returnPartition always returns UNLOCKED partition.
Additionally, the comment says that LOCKED should be returned when a change in the locked partition equals value but it actually checks the change in _partition which is usually UNLOCKED

  1. as mentioned inline please elaborate a bit on the changes.

thanks!
tin/diligence

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 95.181% when pulling d4465e3 on task_3.11 into db6ee34 on dev-3.0.0.

@tintinweb
Copy link

👍 tin/diligence

@maxsam4 maxsam4 changed the base branch from dev-3.0.0 to audit-fixes June 12, 2019 08:00
@maxsam4 maxsam4 merged commit 3176359 into audit-fixes Jun 12, 2019
@maxsam4 maxsam4 deleted the task_3.11 branch June 12, 2019 10:32
adamdossa pushed a commit that referenced this pull request Jun 14, 2019
* Add StatusCode library

* make inline library

* remove the helpers/PolyToken.sol

* remove the IBoot from the contracts (#687)

* cleanup (#686)

* remove unnecessary modifiers (#685)

* Remove KindMath from TokenLib (#684)

* cleanup

* remove the KindMath

* Marked some functions as external (#678)

* Removed redundant pause/unpause (#676)

* Made onlyModuleOrOwner definition consisitent (#674)

* Made onlyModuleOrOwner definition consisitent

* Added braces

* Added braces

* minor undeflow fix in vesting ewallet (#673)

* Updated license to Apache 2.0

* Update LICENSE

* [3.20] Don't ask for name when registering ticker (#706)

* Removed storing token name when registering ticker

* Added backwards compatible functions

* Updated tests to use latest functions

* Renamed functions for easy testing on truffle

* Fixed test

* [3.38]: Add ST storage layout check script in CI pipeline (#704)

* add st storage layout check script in CI pipeline

* add info comment

* 3.30   Remove OwnedProxy (#701)

* Audit fix

* Fix

* Moved variable to storage contract

* [3.31] Removed take usage fee (#700)

* Removed takeUsageFee

* Test fix

* tests fixed

* [3.5, 3.7, 3.8] Fix custom modules (#698)

* Fix custom modules

* Remove unnecessary modifier

* Fix some test cases

* disallow creation of 0 supply dividend (#697)

* Added break in deleteDelegate (#696)

* Added 0 length name check (#695)

* Audit  3.4 & 3.14 Fix ST upgrades (#694)

* Fixes + test cases

* Small change in test

* Add fixes for 3.14

* Fix setProtocolFactory() (#689)

* Remove inconsistency of the index value (#688)

* remove the unneccessary code from partitionsOf() (#681)

* Added constructors (#672)

* Added constructors
The constructors prevents an exploit in case we forget to initialize the implementation contracts.
Ideally, we should allways remember to initialize implementation contracts as well.

* Migration fixed

* test fix

* test fix

* Add the ability to configure the new STR atomically

* Fix the returnPartition function (#680)

* fix the returnPartition function

* minor fix

* Remove comment from codebase (#714)

* Update interfaces to named parameters (#708)

* WIP

* Change some values

* Make some mappings public for automatic getters

* Put back truffle version

* updated transfer manager results (#693)

* Allow Custom oracle in USDTieredSTO (#691)

* Added custom oracles to USDTSTO

* Updated custom oracles logic

* Pinned solidity version (#675)

* Pinned solidity version

* Fix merge conflicts

* minor transfer optimization (#668)

* Fix the BalanceOfPartition audit item (#679)

* fix balance of partition function

* fix balance of Partition function

* return balance in the parent implementation of the getTokensByPartition()

* Extra Items  (#702)

* minor improvements

* permission fixes

* Synchronise the ISTR with STR (#682)

* synchronise the ISTR with STR

* fix the interface problem

* minor fixes

* add some function in interface

* add comment in STR

* consistency in interfaces of contracts

* improve the st interface

* remove shadow declaration

* add #706 new function declartion in the ISTR

* add missing functions in interface

* Specific contract type used (#683)

* specific contract type used

* add more type contract

* remove the redundant KindMath

* remove unnecessary casting

* Merge fixes

* Fix PLCR Proposal inconsistency (#713)

* fix the proposal

* minor styling and comments addition

* Fix contract size issue.

* Update licenses - Apache 2.0

As per https://spdx.org/licenses/

* Fix canTransfer spec (#709)

* Fix spec

* Fix conflict

* Fix merge conflict

* Small fixes

* Revert package.json change

* Fix test case

* Set solcjs as default compiler (#716)

* Set solcjs as default compiler
If you want to use native solc, set the environemnt variable POLYMATH_NATIVE_SOLC as true.

* Removed native solc from travis

* Removed native solc version query from travis
adamdossa pushed a commit that referenced this pull request Jul 30, 2019
* PLCR voting module

* WIP

* Clean up types & tags

* Fix test cases

* Check that there is a non-zero supply when creating a dividend

* improvements and fixes

* factory fix

* minor fix

* improve the PLCR voting

* add more functionality and add test cases

* Ported STO fixes from dev-2.1.0 (#591)

* Ported STO fixes from dev-2.1.0

* Revert when cap reached instead of failing silently

* Restored missing require

* USDTSTO granularity edge case fixed

* add more tests

* add overflow check

* add overflow check

* GTM minor refactoring

* gtm test fix

* MATM optimizations

* Fixed matm tests

* minor fixes

* add comments

* minor comment

* Added a test case

* Minor optimization

* add getTokensByPartition in GTM and BTM

* Minor optimization in verify transfer

* add test for the VRTM getTokensByPartition

* add the pause functionality in the getTokensByPartition()

* Get subset of investors at a checkpoint (#611)

* Updated checkpoint event and optimized st

* Optimized dividends push payment

* Added test

* Minor datastore optimization

* Test fixed

* Add acknowledgement for irrevocable actions using EIP712 (#599)

* Require signed user ack

* Added test

* Small fix

* WIP

* Cleanup

* More cleanup

* Cleaned up tests

* Skip acknowledgement tests in coverage

* Test fix

* Merge fix

* add pause effects in LTM

* Allow to define fee in Poly

* fix test

* Updated factory deployments

* Updated STR tests

* Tests fixed

* Added code comment to oracle

* Add STR tests

* Simplified change currency functions

* Updated STR tests

* Added module factory tests

* Fix timestamp for checkpoints tests

* Cosmetic changes

* Fix timestamp for checkpoints tests

* Skip cp module in coverage

* Port dividend fix (#610)

* port the 100% tax withholding changes

* minor size optimization

* add reclaim functions in every module

* minor fixes

* restrict the changes if dividend is expired

* Allow token name change (#600)

* Allow owner to change name

* Reduce size

* Update ST interface

* Added update name event

* Updated OZ

* Updated storage verification test

* Moved BTM, LTM, Vesting out of experimental

* Typo fix

* Test fix

* minor fixes

* start proposal Id from 1 instead of 0

* BTM optimizations and bug fix

* Bug fix

* Typo fix

* Added clash checker script

* Beutified function selector clash check script

* Throw on finding a clash

* Added circle CI job

* Upgradable tokens (#602)

* wip

* Fixes for migration

* Broken :-(

* Fixes

* Fix tests

* Some more fixes

* Lots more logic

* Add ability to add modules as archived

* Add a test

* some more tests

* Remove test file

* Cleanup some test cases

* Fix module / token versioning

* Fix more tests

* More version checking & fixes

* Remove badtest

* Reduce size of mock contract

* Some updates

* Remove unnecessary constructors

* Updates

* Merge fixes

* Minor update

* Merge fixes

* implement all functions of ERC1410

* LTM optimizations and bug fix

* minor fix

* Vesting escrow wallet optimizations and bug fix

* remove the approvals mapping

* Added test cases for bug fixes

* reduce the ST code size upto 23.84 KB

* remove unnecessary mocks contract and cutdown the ST size till 23.72 KB

* Increased max module types (#636)

Increased the number of module types that the STFactory must check for potential incompatibilities before allowing the token to upgrade.

* add test cases and fix bugs

* resolve conflicts and add test cases

* minor test fixes

* minor test fix

* Increase solidity coverage memory limit

* Raised memory limits of truffle and ganache

* refactoring voting modules & adding functionality

* Refresh / Upgrade tokens (#632)

* Added refresh token function

* Reduced STR size

* Added test cases for refreshToken

* reuse function

* Added refresh token event

* fix compiler error

* Port the VRTM audit fixes (#635)

* port the VRTM audit fixes

* cleanup code

* move voting modules from experimental

* fix test & add test

* fix test for weighted vote module

* add more tests

* increase test coverage

* increase more coverage

* Minor optimizations (#628)

* Optimized datastore batch functions

* fixed ST mock compiler warnings

* Optimized 10^18

* Name null check updated

* GPM optimizations

* CTM optimization

* GTM optimization

* Transfer type made enum

* Added compiler version to supress warning

* Minor STR optimization

* Minor verify transfer optimizations

* Added set bytes function

* Marked initialize function of STR non-payable

* Deleted registry updater

* Added comments for pre computed hashes

* Added more pre computed hashes

* batch functions optimized

* minor ST optimizations

* Can transfer minor optimization

* Reduced mock contract size

* Removed dead code

* Minor changes

* Increased STR compatibility (#640)

* Made STR backwards compatible

* Fixed tests

* Added test case

* Merge fix

* Hardcode version checks

* Fixed pclr voting test

* Revert prettification

This reverts commit 7efad95.

* build fix

* Typo fix

* Fixed tests

* Updated and patched soldiity docgen

* Update changelog for some extent

* Copy dev-2.1.0 CLI into dev-3.0.0

* Fix Errors + add missing functions & Events

* Add ISecurityTokenRegistry to contract abis + fix OZ ERC20 abi

* Fix/Update ST20Generator for V3.0.0.

* Add ISecurityToken to contract abis

* Blue Bull - because I like it

* Add events and some public constant getters to ISecurityToken

* Token Manager CLI Fixes

* revert yarn.lock changes

* more token manager fixes

* More CLI STM and TM fixes

* WIP: More TM CLI fixes and updates

* Update as per PR #669 to master

* TM CLI updates for setting and checking investor flags

* CLI Fixes for remaining transfer manager modules

* Port Combine modify whitelist commands (ref PR #667)

* STO CLI 3.0.0 fixes

* Transfer CLI fixes

* Investor CLI fixes

* dividend manager CLI fixes

* Contract Manager CLI Fixes and updates

* Minor ST20 Generator change

* Permission Manager Cli Fixes

* Pin solc to 0.5.8

* made solc executable

* Transfer managers with version

* usd and poly fees for STR

* Labeled modules
Holder count

* CLI Treasury wallet

* CLI Change token name

* CLI ST Documents

* CLI controller transfer renamed

* CLI partitions and operators

* CLI inputs with limits abstracted to input.js

* Moved OZ from devDependencies to dependencies (#707)

* Approved Audit fixes (#705)

* Add StatusCode library

* make inline library

* remove the helpers/PolyToken.sol

* remove the IBoot from the contracts (#687)

* cleanup (#686)

* remove unnecessary modifiers (#685)

* Remove KindMath from TokenLib (#684)

* cleanup

* remove the KindMath

* Marked some functions as external (#678)

* Removed redundant pause/unpause (#676)

* Made onlyModuleOrOwner definition consisitent (#674)

* Made onlyModuleOrOwner definition consisitent

* Added braces

* Added braces

* minor undeflow fix in vesting ewallet (#673)

* Updated license to Apache 2.0

* Update LICENSE

* [3.20] Don't ask for name when registering ticker (#706)

* Removed storing token name when registering ticker

* Added backwards compatible functions

* Updated tests to use latest functions

* Renamed functions for easy testing on truffle

* Fixed test

* [3.38]: Add ST storage layout check script in CI pipeline (#704)

* add st storage layout check script in CI pipeline

* add info comment

* 3.30   Remove OwnedProxy (#701)

* Audit fix

* Fix

* Moved variable to storage contract

* [3.31] Removed take usage fee (#700)

* Removed takeUsageFee

* Test fix

* tests fixed

* [3.5, 3.7, 3.8] Fix custom modules (#698)

* Fix custom modules

* Remove unnecessary modifier

* Fix some test cases

* disallow creation of 0 supply dividend (#697)

* Added break in deleteDelegate (#696)

* Added 0 length name check (#695)

* Audit  3.4 & 3.14 Fix ST upgrades (#694)

* Fixes + test cases

* Small change in test

* Add fixes for 3.14

* Fix setProtocolFactory() (#689)

* Remove inconsistency of the index value (#688)

* remove the unneccessary code from partitionsOf() (#681)

* Added constructors (#672)

* Added constructors
The constructors prevents an exploit in case we forget to initialize the implementation contracts.
Ideally, we should allways remember to initialize implementation contracts as well.

* Migration fixed

* test fix

* test fix

* Add the ability to configure the new STR atomically

* Fix the returnPartition function (#680)

* fix the returnPartition function

* minor fix

* Remove comment from codebase (#714)

* Update interfaces to named parameters (#708)

* WIP

* Change some values

* Make some mappings public for automatic getters

* Put back truffle version

* updated transfer manager results (#693)

* Allow Custom oracle in USDTieredSTO (#691)

* Added custom oracles to USDTSTO

* Updated custom oracles logic

* Pinned solidity version (#675)

* Pinned solidity version

* Fix merge conflicts

* minor transfer optimization (#668)

* Fix the BalanceOfPartition audit item (#679)

* fix balance of partition function

* fix balance of Partition function

* return balance in the parent implementation of the getTokensByPartition()

* Extra Items  (#702)

* minor improvements

* permission fixes

* Synchronise the ISTR with STR (#682)

* synchronise the ISTR with STR

* fix the interface problem

* minor fixes

* add some function in interface

* add comment in STR

* consistency in interfaces of contracts

* improve the st interface

* remove shadow declaration

* add #706 new function declartion in the ISTR

* add missing functions in interface

* Specific contract type used (#683)

* specific contract type used

* add more type contract

* remove the redundant KindMath

* remove unnecessary casting

* Merge fixes

* Fix PLCR Proposal inconsistency (#713)

* fix the proposal

* minor styling and comments addition

* Fix contract size issue.

* Update licenses - Apache 2.0

As per https://spdx.org/licenses/

* Fix canTransfer spec (#709)

* Fix spec

* Fix conflict

* Fix merge conflict

* Small fixes

* Revert package.json change

* Fix test case

* Set solcjs as default compiler (#716)

* Set solcjs as default compiler
If you want to use native solc, set the environemnt variable POLYMATH_NATIVE_SOLC as true.

* Removed native solc from travis

* Removed native solc version query from travis

* CLI common selectToken inlcuding tokensByDelegate

* Merge Dev-3.0.0 interfaces

* Fix underflow in BlacklistTM (#721)

* CLI Refresh security token

* CLI Permissions updated

* CLI Provider validator regex

* Update changelog after the audit fixes

* Fixed errors

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Issue with forceburn wording and no controllerredeem listed

* CLI _owner issue fixed

* CLI NewSecurityToken event renamed

* Revert "CLI NewSecurityToken event renamed"

This reverts commit 4a7fbbe.

* CLI ST20 granularity support

* Verify transfer on each TM
Show which module failed in a TransferFailure
Messages improved

* Protocol upgrade fixes (#733)

* Update tags, types, lower & upper bounds (#725)

* Make useModule backwards compatible

* Make deployToken backwards compatible (#726)

* Make deployToken backwards compatible

* Small fix for scheduledCheckpoint

* Updated STR interface

* Removed extra event

* Fix interface mismatch

* cleaning up as per the audit recommendation (#722)

* TakeFee was removed.

* ISTR script (#731)

* Added Interface sync  script

* Added interface check script to CI

* return 1 on error

* Use local truffle in scripts

* Removed npx dependency

* minor fixes

* CLI Permission manager refactoring

* Fixed CTM verifyTransfer bug (#736)

* Fixed CTM canTransferBug

* Added test case

* Added comment for devs

* Move some variables / functions to internal (#737)

Move some variables / functions to internal

* CLI Fix

* Minor fix

* CLI Wallet manager

* Added VEW to migrations

* CLI VEW schedules in batches

* Styling

* Added note for valid templates

* Added wallet modules to ST manager

* Fixed bug at checking balance

* fix: getTreasuryWallet function added to DividendCheckpoint.sol

* Minor fixes

* Update with v3 contract addresses

* Add files via upload

* Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants