-
Notifications
You must be signed in to change notification settings - Fork 15
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
Rework to the wrapped token approach #9
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.
Can you consider such set of changes to reduce delta of the code which was already audited?
|
||
address sender = _msgSender(); | ||
|
||
IERC20(_token).safeTransferFrom(sender, address(this), _amount); |
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.
Consider either to make the code safer by adding a check that the swap contract balance increased by expected amount of tokens or to extend the code with a comment that this check is not here because the contract admin must be sure that such tokens are not added for swap.
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.
Sure, added a comment
contracts/SBCWrapper.sol
Outdated
address _token, | ||
uint256 _amount | ||
) internal { | ||
uint256 received = (_amount * tokenRate[_token]) / 1 ether; |
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.
Consider to make another name for the variable since although it reflects amount of tokens to receive instead of accepted tokens, it is not received yet. May it be acquired
?
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.
Yes, sounds like a good naming to me. Thanks.
* @param _to address that will receive the locked tokens on this contract. | ||
*/ | ||
function claimTokens(address _token, address _to) external onlyAdmin { | ||
require(tokenStatus[_token] == TokenStatus.DISABLED, "SBCWrapper: token already swappable"); |
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.
Consider to add a comment that the admin needs to make sure that the claimable token does not have the same storage with a token configured for the swap (the situation when different tokens addresses refers to the same storage contract).
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.
Sure.
The usage flow will be the following:
STAKE
tokens to theSBCWrapper
contractSBCWrapper
to swap his tokens on the newly mintedSBCToken
tokensSBCToken
tokens to theSBCDepositContract
contractdeposit
function to make a final SBC deposit.Steps 1 & 2 can be completed in the same transaction by using permit function or ERC677
transferAndCall
standard in the STAKE token.Steps 3 & 4 can be completed in the same transaction via the ERC677
transferAndCall
function in the wrapped token contract.So, in general, 2 transactions from user will be required.
Design considerations:
enableToken
function.