EARN initial feedback

[denalimarsh]

My initial feedback is primarily focused on design considerations and potential attack vectors. As the codebase is in active development, I decided not to include my feedback on several minor implementation details at this point.

Potential issues

• withdraw(): We iterate over an account’s redeemRecords in O(n) time. As records are user-created at the low cost of 1 CORE per record, a malicious actor could create large numbers of records, leading to an out-of-gas error when we attempt to iterate them.
• _unDelegateWithStrategy does not prioritize undelegating from validators that have dropped out of the active validator set. The current random selection could result in a % of the total staked CORE remaining delegated to inactive validators, undelegating instead from active, yield-earning validators. By reducing capital efficiency, EARN will have a decreased APR.

Considerations

• beforeTurnRound ’s basic rebalancing strategy could result in unexpected changes in the active validator set depending on the selected validators’ EARN delegations and the composition of their respective hybrid scores.
• Selecting validators on behalf of users presents an interesting design tradeoff:
  • a) We centralize delegations in the active validator set, creating a more static/stable active validator set and thereby reducing protocol decentralization
  • b) We reduce EARN’s APY by continuing to delegate CORE to non-active validators, making the product less capital-efficient

Minor

• The UI will probably want getRedeemAmount(address account) in addition to the current getRedeemAmount() which only supports msg.sender
• withdraw(): Should check address(this).balance < amount before deleting user’s record, not after.

[jackcrypto9527]

withdraw() agree there is room to improve gas usage, I added an @OpenIssue in code comments. However, the attacking scenario will not stand because the attacker will only make herself out of gas as the records are stored by each address.

_unDelegateWithStrategy make sense on optimizing the strategy to increase APR. We pick some alternative strategies to fulfill that.

• In afterTurnRound() method, the system will check all validator status and move stakes from inactive ones to active ones (newly elected ones) in the new round.
• We also introduced a manual rebalance method so the operator can move staking on certain cases. E.g. from a jailed validator to another one; or from a low APR validator to a high one.

[jackcrypto9527]

beforeTurnRound The purpose here is that we hope to make the Earn module as subjective as possible to all validators. In order to accomplish that, we adopted this rebalance strategy, which is to keep the staking on each validator as even as possible. Might not be a perfect timing right before the turn round, however the rebalance method can be triggered at different time in the latest design which could mitigate your concerns a bit.

Selecting validator. In the latest implementation, the caller can choose a validator to delegate to when minting stCORE. We will leave the frontend to decide whether to randomly choosing a validator for users or leave that to users, or do it in a mixed way.

[denalimarsh]

withdraw() agree there is room to improve gas usage, I added an @OpenIssue in code comments. However, the attacking scenario will not stand because the attacker will only make herself out of gas as the records are stored by each address.

Even if it's not an attacker, a user could add too many redeemRecords and when they hit the out-of-gas error, their funds will be stuck as they won't be able to withdraw.

[jackcrypto9527]

Make sense. @falcon2048 please take a note.

[falcon2048]

So, what if change the data structure of redeemRecords to a iterable map? It seems that the issue cannot be completely resolved.

[jackcrypto9527]

Let's add a limit to user redeem records, e.g. every user can only keep up to 100 records in the contract.