Skip to content

Commit

Permalink
feat: chainsec fixes (#33)
Browse files Browse the repository at this point in the history 
* fix: use safe erc

* fix: combine accountant functions

* fix: typos

* fix: check min idle

* fix: remove refund bool

* fix: debt allocator comments

* feat: check if accountant set

* feat: remove role

* fix: factory is immutable

* fix: role allowed

* fix: config override

* fix: include tests

* fix: dont import tests

* fix: license

* fix: mapping is internal

* fix: remove config override

* fix: wording

Co-authored-by: FP <[email protected]>

---------

Co-authored-by: FP <[email protected]>
  • Loading branch information
@Schlagonia @fp-crypto
Schlagonia and fp-crypto authored Feb 7, 2024
1 parent 4c52060 commit 6a48f73
Show file tree
Hide file tree
Showing 9 changed files with 333 additions and 116 deletions.
9 changes: 6 additions & 3 deletions ape-config.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,26 @@ dependencies:
- name: openzeppelin
github: OpenZeppelin/openzeppelin-contracts
ref: 4.8.2

- name: yearn-vaults
github: yearn/yearn-vaults-v3
ref: v3.0.1
exclude:
- test/
- test/**/*

- name: tokenized-strategy
github: yearn/tokenized-strategy
ref: v3.0.1
contracts_folder: src
exclude:
- test/
- test/**/*

- name: periphery
github: yearn/tokenized-strategy-periphery
ref: master
contracts_folder: src
exclude:
- test/
- test/**/*

solidity:
import_remapping:
Expand Down
58 changes: 46 additions & 12 deletions contracts/Managers/RoleManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ contract RoleManager is Governance2Step {
mapping(address => VaultConfig) public vaultConfig;
/// @notice Mapping of underlying asset, api version and rating to vault.
mapping(address => mapping(string => mapping(uint256 => address)))
public _assetToVault;
internal _assetToVault;

constructor(
address _governance,
Expand Down Expand Up @@ -153,7 +153,7 @@ contract RoleManager is Governance2Step {
)
});

// Security cna set the max debt for strategies to have.
// Security can set the max debt for strategies to have.
_positions[SECURITY] = Position({
holder: _security,
roles: uint96(Roles.MAX_DEBT_MANAGER)
Expand Down Expand Up @@ -393,20 +393,23 @@ contract RoleManager is Governance2Step {
* @param _vault Address of the vault to set up the accountant for.
*/
function _setAccountant(address _vault) internal virtual {
// Temporarily give this contract the ability to set the accountant.
IVault(_vault).add_role(address(this), Roles.ACCOUNTANT_MANAGER);

// Get the current accountant.
address accountant = getPositionHolder(ACCOUNTANT);

// Set the account on the vault.
IVault(_vault).set_accountant(accountant);
// If there is an accountant set.
if (accountant != address(0)) {
// Temporarily give this contract the ability to set the accountant.
IVault(_vault).add_role(address(this), Roles.ACCOUNTANT_MANAGER);

// Take away the role.
IVault(_vault).remove_role(address(this), Roles.ACCOUNTANT_MANAGER);
// Set the account on the vault.
IVault(_vault).set_accountant(accountant);

// Whitelist the vault in the accountant.
HealthCheckAccountant(accountant).addVault(_vault);
// Take away the role.
IVault(_vault).remove_role(address(this), Roles.ACCOUNTANT_MANAGER);

// Whitelist the vault in the accountant.
HealthCheckAccountant(accountant).addVault(_vault);
}
}

/**
Expand Down Expand Up @@ -586,6 +589,29 @@ contract RoleManager is Governance2Step {
emit RemovedVault(_vault);
}

/**
* @notice Removes a specific role(s) for a `_holder` from the `_vaults`.
* @dev Can be used to remove one specific role or multiple.
* @param _vaults Array of vaults to adjust.
* @param _holder Address who's having a role removed.
* @param _role The role or roles to remove from the `_holder`.
*/
function removeRoles(
address[] calldata _vaults,
address _holder,
uint256 _role
) external virtual onlyGovernance {
address _vault;
for (uint256 i = 0; i < _vaults.length; ++i) {
_vault = _vaults[i];
// Make sure the vault is added to this Role Manager.
require(vaultConfig[_vault].asset != address(0), "vault not added");

// Remove the role.
IVault(_vault).remove_role(_holder, _role);
}
}

/*//////////////////////////////////////////////////////////////
SETTERS
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -799,7 +825,15 @@ contract RoleManager is Governance2Step {
}

/**
* @notice Get the address assigned to the allocator.
* @notice Get the address assigned to be the debt allocator if any.
* @return The address assigned to be the debt allocator if any.
*/
function getDebtAllocator() external view virtual returns (address) {
return getPositionHolder(DEBT_ALLOCATOR);
}

/**
* @notice Get the address assigned to the allocator factory.
* @return The address assigned to the allocator factory.
*/
function getAllocatorFactory() external view virtual returns (address) {
Expand Down
77 changes: 76 additions & 1 deletion contracts/Mocks/MockTokenizedStrategy.sol
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,82 @@
// SPDX-License-Identifier: AGPL-3.0
pragma solidity 0.8.18;

import {MockTokenizedStrategy} from "@yearn-vaults/test/mocks/ERC4626/MockTokenizedStrategy.sol";
import {TokenizedStrategy, ERC20} from "@tokenized-strategy/TokenizedStrategy.sol";

contract MockTokenizedStrategy is TokenizedStrategy {
uint256 public minDebt;
uint256 public maxDebt = type(uint256).max;

// Private variables and functions used in this mock.
bytes32 public constant BASE_STRATEGY_STORAGE =
bytes32(uint256(keccak256("yearn.base.strategy.storage")) - 1);

function strategyStorage() internal pure returns (StrategyData storage S) {
// Since STORAGE_SLOT is a constant, we have to put a variable
// on the stack to access it from an inline assembly block.
bytes32 slot = BASE_STRATEGY_STORAGE;
assembly {
S.slot := slot
}
}

constructor(
address _asset,
string memory _name,
address _management,
address _keeper
) {
// Cache storage pointer
StrategyData storage S = strategyStorage();

// Set the strategy's underlying asset
S.asset = ERC20(_asset);
// Set the Strategy Tokens name.
S.name = _name;
// Set decimals based off the `asset`.
S.decimals = ERC20(_asset).decimals();

// Set last report to this block.
S.lastReport = uint128(block.timestamp);

// Set the default management address. Can't be 0.
require(_management != address(0), "ZERO ADDRESS");
S.management = _management;
S.performanceFeeRecipient = _management;
// Set the keeper address
S.keeper = _keeper;
}

function setMinDebt(uint256 _minDebt) external {
minDebt = _minDebt;
}

function setMaxDebt(uint256 _maxDebt) external {
maxDebt = _maxDebt;
}

function availableDepositLimit(
address
) public view virtual returns (uint256) {
uint256 _totalAssets = strategyStorage().totalIdle;
uint256 _maxDebt = maxDebt;
return _maxDebt > _totalAssets ? _maxDebt - _totalAssets : 0;
}

function availableWithdrawLimit(
address /*_owner*/
) public view virtual returns (uint256) {
return type(uint256).max;
}

function deployFunds(uint256 _amount) external virtual {}

function freeFunds(uint256 _amount) external virtual {}

function harvestAndReport() external virtual returns (uint256) {
return strategyStorage().asset.balanceOf(address(this));
}
}

contract MockTokenized is MockTokenizedStrategy {
uint256 public apr;
Expand Down
69 changes: 39 additions & 30 deletions contracts/accountants/HealthCheckAccountant.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ contract HealthCheckAccountant {
_;
}

modifier onlyVaultOrFeeManagers() {
modifier onlyVaultOrFeeManager() {
_checkVaultOrFeeManager();
_;
}
Expand Down Expand Up @@ -151,29 +151,18 @@ contract HealthCheckAccountant {
) {
require(_feeManager != address(0), "ZERO ADDRESS");
require(_feeRecipient != address(0), "ZERO ADDRESS");
require(
defaultManagement <= MANAGEMENT_FEE_THRESHOLD,
"exceeds management fee threshold"
);
require(
defaultPerformance <= PERFORMANCE_FEE_THRESHOLD,
"exceeds performance fee threshold"
);
require(defaultMaxLoss <= MAX_BPS, "too high");

feeManager = _feeManager;
feeRecipient = _feeRecipient;

defaultConfig = Fee({
managementFee: defaultManagement,
performanceFee: defaultPerformance,
refundRatio: defaultRefund,
maxFee: defaultMaxFee,
maxGain: defaultMaxGain,
maxLoss: defaultMaxLoss
});

emit UpdateDefaultFeeConfig(defaultConfig);
_updateDefaultConfig(
defaultManagement,
defaultPerformance,
defaultRefund,
defaultMaxFee,
defaultMaxGain,
defaultMaxLoss
);
}

/**
Expand Down Expand Up @@ -275,7 +264,7 @@ contract HealthCheckAccountant {
* @dev This is not used to set any of the fees for the specific vault or strategy. Each fee will be set separately.
* @param vault The address of a vault to allow to use this accountant.
*/
function addVault(address vault) external virtual onlyVaultOrFeeManagers {
function addVault(address vault) external virtual onlyVaultOrFeeManager {
// Ensure the vault has not already been added.
require(!vaults[vault], "already added");

Expand All @@ -288,9 +277,7 @@ contract HealthCheckAccountant {
* @notice Function to remove a vault from this accountant's fee charging list.
* @param vault The address of the vault to be removed from this accountant.
*/
function removeVault(
address vault
) external virtual onlyVaultOrFeeManagers {
function removeVault(address vault) external virtual onlyVaultOrFeeManager {
// Ensure the vault has been previously added.
require(vaults[vault], "not added");

Expand Down Expand Up @@ -323,14 +310,36 @@ contract HealthCheckAccountant {
uint16 defaultMaxGain,
uint16 defaultMaxLoss
) external virtual onlyFeeManager {
_updateDefaultConfig(
defaultManagement,
defaultPerformance,
defaultRefund,
defaultMaxFee,
defaultMaxGain,
defaultMaxLoss
);
}

/**
* @dev Updates the Accountant's default fee config.
* Is used during deployment and during any future updates.
*/
function _updateDefaultConfig(
uint16 defaultManagement,
uint16 defaultPerformance,
uint16 defaultRefund,
uint16 defaultMaxFee,
uint16 defaultMaxGain,
uint16 defaultMaxLoss
) internal virtual {
// Check for threshold and limit conditions.
require(
defaultManagement <= MANAGEMENT_FEE_THRESHOLD,
"exceeds management fee threshold"
"management fee threshold"
);
require(
defaultPerformance <= PERFORMANCE_FEE_THRESHOLD,
"exceeds performance fee threshold"
"performance fee threshold"
);
require(defaultMaxLoss <= MAX_BPS, "too high");

Expand Down Expand Up @@ -373,11 +382,11 @@ contract HealthCheckAccountant {
// Check for threshold and limit conditions.
require(
customManagement <= MANAGEMENT_FEE_THRESHOLD,
"exceeds management fee threshold"
"management fee threshold"
);
require(
customPerformance <= PERFORMANCE_FEE_THRESHOLD,
"exceeds performance fee threshold"
"performance fee threshold"
);
require(customMaxLoss <= MAX_BPS, "too high");

Expand Down Expand Up @@ -583,8 +592,8 @@ contract HealthCheckAccountant {
uint256 _amount
) internal {
if (ERC20(_token).allowance(address(this), _contract) < _amount) {
ERC20(_token).approve(_contract, 0);
ERC20(_token).approve(_contract, _amount);
ERC20(_token).safeApprove(_contract, 0);
ERC20(_token).safeApprove(_contract, _amount);
}
}

Expand Down
Loading

0 comments on commit 6a48f73

Please sign in to comment.