-
Notifications
You must be signed in to change notification settings - Fork 31
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
Mint ost prime as base token #559
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contracts/gateway/OSTPrime.sol
Outdated
* Acquire lock for msg.sender so that this function can only be | ||
* executed once in a transaction. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contracts/lib/Mutex.sol
Outdated
); | ||
|
||
locks[msg.sender] = true; | ||
success_ = true; |
There was a problem hiding this comment.
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
mutex.releaseExternal(address), | ||
'Lock is not acquired for the address.' | ||
); | ||
}); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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') 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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.
…and contract licence
…vesh-ost/mosaic-contracts into mint_ost_prime_as_base_token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Available for review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎢
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #545
This PR implements the following features:
increaseSupply
transfers base token to the beneficiary.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: