Add MigrationZapper #432
Add MigrationZapper #432
Conversation
|
|
||
| // Stake on behalf of user | ||
| ognStaking.stake( | ||
| newStakeAmount, |
There was a problem hiding this comment.
- The ognReceived might be greater than the newStakeAmount, the difference should be sent back to msg.sender.
- If some ogn are "staying" on the contract, anyone could stake it on his behalf using a really small amount of ogv to migrate and a larger stake amount to swipe the balance of the contract.
There was a problem hiding this comment.
Ah, yes. Good catch
There was a problem hiding this comment.
This is fixed as well
contracts/MigrationZapper.sol
Outdated
| IMigrator public immutable migrator; | ||
| IStaking public immutable ognStaking; | ||
|
|
||
| constructor(address _ogv, address _ogn, address _migrator, address _ognStaking) { |
There was a problem hiding this comment.
If you are willing to add ownership to this contract, I suggest you add a function to recover funds sent by mistake with an onlyOwner modifier.
|
|
||
| // Migrate | ||
| uint256 ognReceived = migrator.migrate(ogvAmount); | ||
|
|
There was a problem hiding this comment.
we could add a:
require(ognReceived >= newStakeAmount, "Not enough OGN received");
Afaik ogn/ogv conversion rates should be locked, and such check should only be triggered by faulty math. Though it is still nice to have an understandable error message
There was a problem hiding this comment.
Good point, we were just running into this error (DApp was sending a higher amount due to a rounding issue)
| governor = _governor; | ||
| } | ||
|
|
||
| function initialize() external { |
There was a problem hiding this comment.
Fortunately, initialize can't do any damage, so it's okay if it's called multiple times, but this should have an initializer modifier.
There was a problem hiding this comment.
Alternatively, if no proxy, this initialize code could move into constructor.
| revert NotGovernor(); | ||
| } | ||
|
|
||
| IMintableERC20(token).transfer(governor, amount); |
There was a problem hiding this comment.
Nobody should send us USDC, but a generic token transfer method should usually use safeMath, since this call will fail on USDC, anything else that does not return a boolean on transfer.
Right now, it's impossible to convert OGV to OGN and stake it in a single call if you don't have any OGV lockups with our Migrator contract.
The new
MigrationZapperwill allow users to do that. It internally callsMigrator.migrateand then stakes it on xOGN contract.The simple
migrate(uint256)should:The other
migrate(uint256,uint256,uint256)should:If you made a contract change, make sure to complete the checklist below before merging it in master.
Contract change checklist: