Skip to content
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

first version of fast-withdrawal feature is ready #24

Open
wants to merge 5 commits into
base: pa
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 49 additions & 7 deletions src/contracts/IStakeToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,34 @@
pragma solidity ^0.8.0;

interface IStakeToken {
struct CooldownSnapshot {
uint40 timestamp;
struct CooldownSetup {
pavelvm5 marked this conversation as resolved.
Show resolved Hide resolved
// make more sense to display time from which can be withdrawn, not activation time
pavelvm5 marked this conversation as resolved.
Show resolved Hide resolved
/// @notice The time after which funds can be redeemed
uint32 timestamp;
pavelvm5 marked this conversation as resolved.
Show resolved Hide resolved
/// @notice The amount of tokens which can be redeemed
uint216 amount;
}

struct SmConfig {
/// @notice Seconds available to redeem once the cooldown period is fulfilled
uint32 unstakeWindowSeconds;
/// @notice Seconds between starting cooldown and being able to withdraw
uint32 cooldownSeconds;
uint32 defaultCooldownSeconds;
/// @notice The address of the underlying asset
address stakedToken;
// reserved for future use
/// @notice The maximum time available for reduction cooldown period
uint32 maxReductionSeconds;
pavelvm5 marked this conversation as resolved.
Show resolved Hide resolved
/// @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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in most other contracts we handle the treasury as immutable as there's no good reason for it to ever change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

event Cooldown(address indexed user, uint256 amount);
event Cooldown(address indexed user, uint256 amount, uint32 timeToRedeem);
event FeesSentToTreasury(uint256 amount);
event TreasuryChanged(address treasury);
event MaxFeeChanged(uint256 maxFee);
event MaxReductionSecondsChanged(uint256 maxReductionSeconds);

event Staked(address indexed from, address indexed to, uint256 assets, uint256 shares);
event Redeem(address indexed from, address indexed to, uint256 assets, uint256 shares);
Expand Down Expand Up @@ -51,6 +63,12 @@ interface IStakeToken {
*/
function cooldown() external;

/**
* @dev Activates the cooldown period and reduces it
* @param reducedTime Time by which the cooldown will be reduced
*/
function reducedCooldown(uint256 reducedTime) external;

/**
* @dev Allows staking a certain amount of STAKED_TOKEN with gasless approvals (permit)
* @param amount The amount to be staked
Expand Down Expand Up @@ -88,9 +106,15 @@ interface IStakeToken {

/**
* @dev Getter of the cooldown seconds
* @return cooldownSeconds the amount of seconds between starting the cooldown and being able to redeem
* @return defaultCooldownSeconds The amount of seconds between starting the cooldown and being able to redeem by default
*/
function getDefaultCooldownSeconds() external view returns (uint256);

/**
* @dev Getter of the maximum reduction cooldown seconds
* @return maxReductionSeconds The maximum time available for reduction cooldown period
*/
function getCooldownSeconds() external view returns (uint256);
function getMaxReductionSeconds() external view returns (uint256);

/**
* @dev Setter of cooldown seconds
Expand All @@ -99,6 +123,24 @@ interface IStakeToken {
*/
function setCooldownSeconds(uint256 cooldownSeconds) external;

/**
* @dev Setter of treasury address
* @param treasury new treasury address to set for collecting fast-withdrawal fees
*/
function setTreasury(address treasury) external;

/**
* @dev Setter of max fee
* @param maxFee Amount of fees in BPS, which should be paid for max cooldown reduction
*/
function setMaxFee(uint256 maxFee) external;

/**
* @dev Setter of max cooldown reduction time in seconds
* @param newMaxReductionTime number of seconds by which the cooldown can be reduced with the payment of fees
*/
function setMaxReductionSeconds(uint256 newMaxReductionTime) external;

/**
* @dev returns the exact amount of shares that would be received for the provided number of assets
* @param assets the number of assets to stake
Expand Down
172 changes: 143 additions & 29 deletions src/contracts/StakeToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {

uint216 public constant INITIAL_EXCHANGE_RATE = 1e18;
uint256 public constant EXCHANGE_RATE_UNIT = 1e18;
uint216 public constant HUNDRED_PERCENT = 10_000;

IRewardsController public immutable REWARDS_CONTROLLER;

/// @custom:storage-location erc7201:aave.storage.StakeToken
struct StakeTokenStorage {
mapping(address => CooldownSnapshot) _stakersCooldowns;
mapping(address => CooldownSetup) _stakersCooldowns;
SmConfig _smConfig;
/// @notice Current exchangeRate of the stk
uint216 _currentExchangeRate;
Expand All @@ -54,7 +55,7 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
}
}

function stakersCooldowns(address user) public view returns (CooldownSnapshot memory) {
function stakersCooldowns(address user) public view returns (CooldownSetup memory) {
StakeTokenStorage storage $ = _getStakeTokenStorage();
return $._stakersCooldowns[user];
}
Expand All @@ -70,18 +71,29 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
string calldata symbol,
address newSlashingAdmin,
uint256 cooldownSeconds,
uint256 unstakeWindow
uint256 unstakeWindow,
address treasury,
uint256 maxFee,
uint256 maxReductionSeconds
) external virtual initializer {
StakeTokenStorage storage $ = _getStakeTokenStorage();
$._smConfig.stakedToken = stakedToken;
// @pavelvm5 made like this, due to stack-too-deep error, will fix in future, if any args will be added could be useful to rewrite to assembly
_getStakeTokenStorage()._smConfig.stakedToken = stakedToken;
_getStakeTokenStorage()._minAssetsRemaining = 10 ** decimals();

__ERC20_init(name, symbol); // TODO: should naming be inherited from underlying or not?
__Ownable_init(newSlashingAdmin);
__EIP712_init(string(abi.encodePacked('stk', name)), '1');

_setSlashingAdmin(newSlashingAdmin);
_setTreasury(treasury);

_setMaxFee(maxFee);

_setCooldownSeconds(cooldownSeconds);
_setUnstakeWindow(unstakeWindow);
_setMaxReductionSeconds(maxReductionSeconds);

_updateExchangeRate(INITIAL_EXCHANGE_RATE);
$._minAssetsRemaining = 10 ** decimals();
}

function decimals() public view override returns (uint8) {
Expand All @@ -104,8 +116,11 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
}

function _setUnstakeWindow(uint256 newUnstakeWindow) internal {
require(newUnstakeWindow >= 1 hours, 'TOO_LOW_UNSTAKE_WINDOW');
pavelvm5 marked this conversation as resolved.
Show resolved Hide resolved

StakeTokenStorage storage $ = _getStakeTokenStorage();
$._smConfig.unstakeWindowSeconds = newUnstakeWindow.toUint32();

emit UnstakeWindowChanged(newUnstakeWindow);
}

Expand Down Expand Up @@ -169,16 +184,23 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {

/// @inheritdoc IStakeToken
function cooldownOnBehalfOf(address from) external onlyOwner {
// @audit-info same modif here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't understand that comment, can you elaborate?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_cooldown(from);
}

/// @inheritdoc IStakeToken
function reducedCooldown(uint256 reductionTime) external {
_reducedCooldown(msg.sender, reductionTime.toUint32());
}

/// @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in aave contracts owner is always governance short exec, imo doesn't make sense to change the naming now

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_redeem(from, to, amount.toUint104());
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i personally agree, but would say it's to opinionated - better to follow the standard.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// @inheritdoc IStakeToken
function getCooldownSeconds() external view returns (uint256) {
function setTreasury(address treasury) external onlyOwner {
_setTreasury(treasury);
}

function _setTreasury(address newTreasury) internal {
require(newTreasury != address(0), 'INVALID_TREASURY_ADDRESS'); // @audit-info-gas/bytecode-optimization @pavelvm5 propose to change from require(true) to if(false) revert ERROR_NAME(), cause its more optimized

_getStakeTokenStorage()._smConfig.treasury = newTreasury;

emit TreasuryChanged(newTreasury);
}

/// @inheritdoc IStakeToken
function setMaxFee(uint256 maxFee) external onlyOwner {
_setMaxFee(maxFee);
}

function _setMaxFee(uint256 newMaxFee) internal {
require(newMaxFee.toUint216() <= HUNDRED_PERCENT && newMaxFee > 0, 'INVALID_MAX_FEE_PARAMETER');

_getStakeTokenStorage()._smConfig.maxFee = newMaxFee.toUint216();

emit MaxFeeChanged(newMaxFee);
}

/// @inheritdoc IStakeToken
function setMaxReductionSeconds(uint256 newMaxReductionTime) external onlyOwner {
_setMaxReductionSeconds(newMaxReductionTime);
}

function _setMaxReductionSeconds(uint256 newMaxReductionSeconds) internal {
StakeTokenStorage storage $ = _getStakeTokenStorage();
return $._smConfig.cooldownSeconds;
SmConfig memory smConfig = $._smConfig;

require(
newMaxReductionSeconds <= smConfig.defaultCooldownSeconds,
'INVALID_MAX_REDUCTION_TIME'
);

_getStakeTokenStorage()._smConfig.maxReductionSeconds = newMaxReductionSeconds.toUint32();

emit MaxReductionSecondsChanged(newMaxReductionSeconds);
}

/// @inheritdoc IStakeToken
function getDefaultCooldownSeconds() external view returns (uint256) {
StakeTokenStorage storage $ = _getStakeTokenStorage();
return $._smConfig.defaultCooldownSeconds;
}

/// @inheritdoc IStakeToken
function getMaxReductionSeconds() public view returns (uint256) {
StakeTokenStorage storage $ = _getStakeTokenStorage();

return $._smConfig.maxReductionSeconds;
}

function _cooldown(address from) internal {
uint256 amount = balanceOf(from);

require(amount != 0, 'INVALID_BALANCE_ON_COOLDOWN');

StakeTokenStorage storage $ = _getStakeTokenStorage();

uint32 timeToRedeem = (block.timestamp + $._smConfig.defaultCooldownSeconds).toUint32();

$._stakersCooldowns[from] = CooldownSetup({
timestamp: timeToRedeem,
amount: amount.toUint216()
});

emit Cooldown(from, amount, timeToRedeem);
}

function _reducedCooldown(address from, uint32 reductionTime) internal {
pavelvm5 marked this conversation as resolved.
Show resolved Hide resolved
StakeTokenStorage storage $ = _getStakeTokenStorage();
$._stakersCooldowns[from] = CooldownSnapshot({
timestamp: uint40(block.timestamp),
amount: uint216(amount)
SmConfig memory smConfig = $._smConfig;

require(reductionTime <= smConfig.maxReductionSeconds, 'MAX_REDUCTION_TIME_EXCEEDED');

uint216 amount = balanceOf(from).toUint216();
require(amount != 0, 'INVALID_BALANCE_ON_COOLDOWN');

uint216 feeAmount = (amount * smConfig.maxFee * reductionTime) /
smConfig.maxReductionSeconds /
HUNDRED_PERCENT;

uint32 timeToRedeem = (block.timestamp + smConfig.defaultCooldownSeconds - reductionTime)
.toUint32();

// @pavelvm5 move to internal block, cause it's logically responsible for transferring fees to treasury, don't want to make another internal function
{
uint256 underlyingForTreasury = previewRedeem(feeAmount);

_burn(from, feeAmount);

IERC20(smConfig.stakedToken).safeTransfer(smConfig.treasury, underlyingForTreasury);

emit FeesSentToTreasury(underlyingForTreasury);
}

$._stakersCooldowns[from] = CooldownSetup({
timestamp: timeToRedeem,
amount: amount - feeAmount
});

emit Cooldown(from, amount);
emit Cooldown(from, amount, timeToRedeem);
}

/**
Expand All @@ -256,7 +371,7 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
*/
function _setCooldownSeconds(uint256 cooldownSeconds) internal {
pavelvm5 marked this conversation as resolved.
Show resolved Hide resolved
StakeTokenStorage storage $ = _getStakeTokenStorage();
$._smConfig.cooldownSeconds = cooldownSeconds.toUint32();
$._smConfig.defaultCooldownSeconds = cooldownSeconds.toUint32();
emit CooldownSecondsChanged(cooldownSeconds);
}

Expand All @@ -271,7 +386,7 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
uint256 sharesToMint = previewStake(amount);
require(sharesToMint != 0, 'INVALID_ZERO_AMOUNT_AFTER_CONVERSION');

_mint(to, sharesToMint.toUint104());
_mint(to, sharesToMint);

StakeTokenStorage storage $ = _getStakeTokenStorage();
IERC20($._smConfig.stakedToken).safeTransferFrom(from, address(this), amount);
Expand All @@ -289,20 +404,17 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
require(amount != 0, 'INVALID_ZERO_AMOUNT');

StakeTokenStorage storage $ = _getStakeTokenStorage();
CooldownSnapshot memory cooldownSnapshot = $._stakersCooldowns[from];
CooldownSetup memory cooldownSetup = $._stakersCooldowns[from];
SmConfig memory cachedSmConfig = $._smConfig;

require(block.timestamp >= cooldownSetup.timestamp, 'INSUFFICIENT_COOLDOWN');
require(
(block.timestamp >= cooldownSnapshot.timestamp + cachedSmConfig.cooldownSeconds),
'INSUFFICIENT_COOLDOWN'
);
require(
(block.timestamp - (cooldownSnapshot.timestamp + cachedSmConfig.cooldownSeconds) <=
cachedSmConfig.unstakeWindowSeconds),
block.timestamp - cooldownSetup.timestamp <= cachedSmConfig.unstakeWindowSeconds,
'UNSTAKE_WINDOW_FINISHED'
);

uint256 maxRedeemable = cooldownSnapshot.amount;
require(maxRedeemable != 0, 'INVALID_ZERO_MAX_REDEEMABLE');
uint256 maxRedeemable = cooldownSetup.amount;
require(maxRedeemable != 0, 'INVALID_ZERO_MAX_REDEEMABLE'); // @audit-info @pavelvm5 need to check this, I think it's unreachable

uint256 amountToRedeem = (amount > maxRedeemable) ? maxRedeemable : amount;

Expand All @@ -329,15 +441,15 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
/**
* @dev calculates the exchange rate based on totalAssets and totalShares
* @dev always rounds up to ensure 100% backing of shares by rounding in favor of the contract
* @param totalAssets The total amount of assets staked
* @param totalShares The total amount of shares
* @param _totalAssets The total amount of assets staked
* @param _totalShares The total amount of shares
* @return exchangeRate as 18 decimal precision uint216
*/
function _getExchangeRate(
uint256 totalAssets,
uint256 totalShares
uint256 _totalAssets,
uint256 _totalShares
) internal pure returns (uint216) {
return (((totalShares * EXCHANGE_RATE_UNIT) + totalAssets - 1) / totalAssets).toUint216();
return (((_totalShares * EXCHANGE_RATE_UNIT) + _totalAssets - 1) / _totalAssets).toUint216(); // @audit-info @pavelvm5 shadow declaration warning here, changed names
}

function _update(address from, address to, uint256 amount) internal override {
Expand All @@ -353,12 +465,14 @@ contract StakeToken is ERC20PermitUpgradeable, IStakeToken, Rescuable {
// Sender
REWARDS_CONTROLLER.handleAction(from, cachedTotalSupply, balanceOfFrom);
StakeTokenStorage storage $ = _getStakeTokenStorage();
CooldownSnapshot memory previousSenderCooldown = $._stakersCooldowns[from];
CooldownSetup memory previousSenderCooldown = $._stakersCooldowns[from];
if (previousSenderCooldown.timestamp != 0) {
// update to 0 means redeem
// this is based on the assumption that erc20 forbids transfer to 0
if (to == address(0)) {
// @audit-info @pavelvm5 can modify this place for optimization, cause sending fees to treasure and burning inside reducedCooldown will go here and this place will be overwritten in any way
if (previousSenderCooldown.amount <= amount) {
// @audit-info @pavelvm5 should be == here, cause we shouldn't be able to redeem more than we have in cooldown, we can add this check
delete $._stakersCooldowns[from];
} else {
$._stakersCooldowns[from].amount = uint216(previousSenderCooldown.amount - amount);
Expand Down
Loading
Loading