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

Mint ost prime as base token #559

Merged
merged 14 commits into from
Dec 21, 2018

Conversation

0xsarvesh
Copy link
Contributor

Fixes #545

This PR implements the following features:

  1. OST prime increaseSupply transfers base token to the beneficiary.
  2. Mutex contract.
  3. Mutex locking for increaseSupply

Mutex is implemented in increaseSupply to avoid possible Reentrancy Attack..
In OSTPrime.increaseSupply(), address.transfer() is used to send base tokens. transfer method ensure that only 2300 gas is passed as "gas stipend" to the payable fallback function of attacker contract. This makes it difficult for an attacker to call any method which updates storage from fallback.

Theoretically, it's almost impossible to do reentrancy attack on increase supply.
For the safer side mutex locking is implemented for increaseSupply as solidity document also mention that 2300 gas "gas stipend" can possibly change in future.

Quote:

During the execution of the fallback function, the contract can only rely on the “gas stipend” it is passed (2300 gas) being available to it at that time. This stipend is not enough to modify storage (do not take this for granted though, the stipend might change with future hard forks).

Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

Very, very cool
image

I like!

Some comments and questions.

* Acquire lock for msg.sender so that this function can only be
* executed once in a transaction.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

contracts/gateway/OSTPrime.sol Outdated Show resolved Hide resolved
contracts/lib/Mutex.sol Outdated Show resolved Hide resolved
contracts/lib/Mutex.sol Outdated Show resolved Hide resolved
);

locks[msg.sender] = true;
success_ = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe isAcquired_? Required?

test/lib/mutex/release.js Outdated Show resolved Hide resolved
mutex.releaseExternal(address),
'Lock is not acquired for the address.'
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add it('should only release the lock of the given address') (and not any other locked address).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it not the same as this test `it('should release lock for an address') 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

You are not checking that it does not release the lock of another address at the same time.

test/lib/mutex/acquire.js Outdated Show resolved Hide resolved
test/lib/mutex/acquire.js Outdated Show resolved Hide resolved
test/lib/mutex/release.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

Available for the review.

@schemar I have responded to your comments/question. Please have a look.

Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

Nice work 🍹
Few small comments.

contracts/lib/MutexAddress.sol Show resolved Hide resolved
contracts/lib/MutexAddress.sol Outdated Show resolved Hide resolved
contracts/test/TestMutexAddress.sol Outdated Show resolved Hide resolved
contracts/test/TestMutexAddress.sol Outdated Show resolved Hide resolved
test/gateway/ost_prime/increase_supply.js Outdated Show resolved Hide resolved
test/gateway/ost_prime/increase_supply.js Outdated Show resolved Hide resolved
test/lib/mutex/acquire.js Show resolved Hide resolved
test/lib/mutex/acquire.js Show resolved Hide resolved
@0xsarvesh 0xsarvesh removed the gateway label Dec 19, 2018
Copy link
Contributor Author

@0xsarvesh 0xsarvesh left a comment

Choose a reason for hiding this comment

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

Available for review.

Copy link
Contributor

@deepesh-kn deepesh-kn left a comment

Choose a reason for hiding this comment

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

LGTM 🎢

Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

Almost 🐤
Just a few remaining questions.

Copy link
Contributor

@schemar schemar left a comment

Choose a reason for hiding this comment

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

LGTM
image

@deepesh-kn deepesh-kn merged commit 6672548 into OpenST:develop Dec 21, 2018
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.

4 participants