first version of fast-withdrawal feature is ready #24
Conversation
| /// @notice The maximum fee in BIPS that will be taken when the cooldown period is reduced by maxReductionTime | ||
| uint216 maxFee; | ||
| /// @notice The address of treasury | ||
| address treasury; |
There was a problem hiding this comment.
in most other contracts we handle the treasury as immutable as there's no good reason for it to ever change
There was a problem hiding this comment.
I added a setter here because the possibility of distributing these rewards to users as additional APY was previously discussed.
So I decided to leave some flexibility here.
src/contracts/StakeToken.sol
Outdated
| @@ -169,16 +184,23 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable { | |||
|
|
|||
| /// @inheritdoc IStakeToken | |||
| function cooldownOnBehalfOf(address from) external onlyOwner { | |||
| // @audit-info same modif here | |||
There was a problem hiding this comment.
don't understand that comment, can you elaborate?
There was a problem hiding this comment.
The comment refers to the same division of roles as stated below.
It is not clear why the owner is able to move my funds.
| /// @inheritdoc IStakeToken | ||
| function redeem(address to, uint256 amount) external { | ||
| _redeem(msg.sender, to, amount.toUint104()); | ||
| } | ||
|
|
||
| /// @inheritdoc IStakeToken | ||
| function redeemOnBehalf(address from, address to, uint256 amount) external onlyOwner { | ||
| // @audit-info why we have onlyOwner here, it looks like rug, we should set different role here |
There was a problem hiding this comment.
in aave contracts owner is always governance short exec, imo doesn't make sense to change the naming now
There was a problem hiding this comment.
I think it’s a good pattern to have 2 different roles that will be responsible for helping/calling their methods.
Being an auditor or any third party not familiar with the specifics of implementation in AAVE, I will have questions about why the owner (regardless of whether it is a DAO or some kind of contract or address) can withdraw my money specifically to any address.
| @@ -232,22 +254,115 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable { | |||
| _setCooldownSeconds(cooldownSeconds); | |||
| } | |||
|
|
|||
| // TODO: @pavelvm5 wouldn't sort functions here cause it can cause conflicts, but standard is external - first, public .., internal, makes easier to understand everything | |||
There was a problem hiding this comment.
i personally agree, but would say it's to opinionated - better to follow the standard.
There was a problem hiding this comment.
It's absolutely unreadable to have such scattered functions. It’s just difficult for me to understand what entry points the contract has and what methods are available inside.
I don’t consider this opinioned, at least because out of 40 projects I have never seen anyone write this way. This is at least some informations about a bad style guide and unreadable code.
Fast-withdrawal feature - first version (without liquidity accounting).
Added some new tests, changed old ones, which don't suit current implementation.