Skip to content

Remove unused artifacts, fix coverage#148

Merged
EdNoepel merged 11 commits intov3from
ed/pe-2585
May 26, 2025
Merged

Remove unused artifacts, fix coverage#148
EdNoepel merged 11 commits intov3from
ed/pe-2585

Conversation

@EdNoepel
Copy link
Contributor

@EdNoepel EdNoepel commented May 22, 2025

Removed artifacts per ticket. Rolled Pausable into Mutator. Implemented missing Attribute and Mutable tests.

@@ -1,5 +1,5 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.8.25 <0.9.0;
pragma solidity ^0.8.20;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was breaking coverage

@github-actions
Copy link

github-actions bot commented May 22, 2025

Slither report

Static Analysis Report**THIS CHECKLIST IS NOT COMPLETE**. Use `--show-ignored-findings` to show all the results. Summary - [locked-ether](#locked-ether) (1 results) (Medium) - [reentrancy-no-eth](#reentrancy-no-eth) (2 results) (Medium) - [unused-return](#unused-return) (8 results) (Medium) - [incorrect-modifier](#incorrect-modifier) (1 results) (Low) - [calls-loop](#calls-loop) (2 results) (Low) - [reentrancy-benign](#reentrancy-benign) (3 results) (Low) - [reentrancy-events](#reentrancy-events) (4 results) (Low) - [dead-code](#dead-code) (44 results) (Informational) - [solc-version](#solc-version) (3 results) (Informational) - [incorrect-using-for](#incorrect-using-for) (1 results) (Informational) - [missing-inheritance](#missing-inheritance) (2 results) (Informational) - [naming-convention](#naming-convention) (13 results) (Informational) - [unimplemented-functions](#unimplemented-functions) (4 results) (Informational) ## locked-ether Impact: Medium Confidence: High - [ ] ID-0 Contract locking ether found: Contract [MutablePauseTarget](https://github.com/equilibria-xyz/root/blob/9955006221ec660d8ef3a85f6503e0757ad17a45/src/mutability/Mutable.sol#L132-L137) has payable functions: - [MutablePauseTarget.fallback()](https://github.com/equilibria-xyz/root/blob/9955006221ec660d8ef3a85f6503e0757ad17a45/src/mutability/Mutable.sol#L133-L135) But does not have a function to withdraw the ether

https://github.com/equilibria-xyz/root/blob/9955006221ec660d8ef3a85f6503e0757ad17a45/src/mutability/Mutable.sol#L132-L137

reentrancy-no-eth

Impact: Medium
Confidence: Medium

function _upgrade(IImplementation newImplementation, bytes memory data) private {
// validate the upgrade metadata of the new implementation
if (
(_implementation() == address(0) ? VersionLib.from(0, 0, 0) : IImplementation(_implementation()).version())
!= newImplementation.predecessor()
) revert MutablePredecessorMismatch();
if (newImplementation.version() == Mutable$().version) revert MutableVersionMismatch();
// update the implementation and call its constructor
ERC1967Utils.upgradeToAndCall(address(newImplementation), abi.encodeCall(IImplementation.construct, (data)));
// record the new implementation version
Mutable$().version = newImplementation.version();
}

function _unpause() private whenPaused {
ERC1967Utils.upgradeToAndCall(Mutable$().paused, "");
Mutable$().paused = address(0);
emit Unpaused();
}

unused-return

Impact: Medium
Confidence: Medium

function approve(Token6 self, address grantee) internal {
IERC20(Token6.unwrap(self)).approve(grantee, type(uint256).max);
}

function approve(Token self, address grantee, uint256 amount) internal {
IERC20(Token.unwrap(self)).approve(grantee, amount);
}

function approve(Token18 self, address grantee, UFixed18 amount) internal {
IERC20(Token18.unwrap(self)).approve(grantee, UFixed18.unwrap(amount));
}

function approve(Token6 self, address grantee, UFixed6 amount) internal {
IERC20(Token6.unwrap(self)).approve(grantee, UFixed6.unwrap(amount));
}

function addDistributions(Token18 token, bytes32 merkleRoot) external override onlyOwner {
if (_merkleRoots.contains(merkleRoot)) revert AirdropDistributionAlreadyExists();
distributions[merkleRoot] = token;
_merkleRoots.add(merkleRoot);
emit DistributionAdded(token, merkleRoot);
}

function create(
IImplementation implementation,
bytes calldata data
) public onlyOwner returns (IMutableTransparent newMutable) {
_mutables.add(address(newMutable = new Mutable(implementation, data)));
_nameToMutable[ShortStrings.toShortString(implementation.name())] = IMutable(address(newMutable));
// ensure state of new mutable is consistent with mutator
if (paused()) IMutable(address(newMutable)).pause();
}

function approve(Token self, address grantee) internal {
IERC20(Token.unwrap(self)).approve(grantee, type(uint256).max);
}

function approve(Token18 self, address grantee) internal {
IERC20(Token18.unwrap(self)).approve(grantee, type(uint256).max);
}

incorrect-modifier

Impact: Low
Confidence: High

modifier initializer(string memory attribute) {
if (!_constructing()) revert AttributeNotConstructing();
if (!Attribute$().attributes[attribute]) _;
Attribute$().attributes[attribute] = true;
}

calls-loop

Impact: Low
Confidence: Medium

function _pause() internal override {
for (uint256 i = 0; i < _mutables.length(); i++) IMutable(_mutables.at(i)).pause();
super._pause();
}

function _unpause() internal override {
for (uint256 i = 0; i < _mutables.length(); i++) IMutable(_mutables.at(i)).unpause();
super._unpause();
}

reentrancy-benign

Impact: Low
Confidence: Medium

function _unpause() internal override {
for (uint256 i = 0; i < _mutables.length(); i++) IMutable(_mutables.at(i)).unpause();
super._unpause();
}

function _pause() internal override {
for (uint256 i = 0; i < _mutables.length(); i++) IMutable(_mutables.at(i)).pause();
super._pause();
}

function create(
IImplementation implementation,
bytes calldata data
) public onlyOwner returns (IMutableTransparent newMutable) {
_mutables.add(address(newMutable = new Mutable(implementation, data)));
_nameToMutable[ShortStrings.toShortString(implementation.name())] = IMutable(address(newMutable));
// ensure state of new mutable is consistent with mutator
if (paused()) IMutable(address(newMutable)).pause();
}

reentrancy-events

Impact: Low
Confidence: Medium

function _unpause() private whenPaused {
ERC1967Utils.upgradeToAndCall(Mutable$().paused, "");
Mutable$().paused = address(0);
emit Unpaused();
}

function _pause() internal override {
for (uint256 i = 0; i < _mutables.length(); i++) IMutable(_mutables.at(i)).pause();
super._pause();
}

function _unpause() internal override {
for (uint256 i = 0; i < _mutables.length(); i++) IMutable(_mutables.at(i)).unpause();
super._unpause();
}

function _pause() private whenUnpaused {
Mutable$().paused = _implementation();
ERC1967Utils.upgradeToAndCall(_pauseTarget, "");
emit Paused();
}

dead-code

Impact: Informational
Confidence: Medium

function eq(UFixed18 a, UFixed18 b) pure returns (bool) {
return UFixed18.unwrap(a) == UFixed18.unwrap(b);
}

function add(Fixed6 a, Fixed6 b) pure returns (Fixed6) {
return Fixed6.wrap(Fixed6.unwrap(a) + Fixed6.unwrap(b));
}

function eq(UFixed6 a, UFixed6 b) pure returns (bool) {
return UFixed6.unwrap(a) == UFixed6.unwrap(b);
}

  • ID-24
    neg(Fixed6) is never used and should be removed

function neg(Fixed6 a) pure returns (Fixed6) {
return Fixed6.wrap(-Fixed6.unwrap(a));
}

function _constructing() internal view override returns (bool) {
return Implementation$().constructing;
}

function add(Fixed18 a, Fixed18 b) pure returns (Fixed18) {
return Fixed18.wrap(Fixed18.unwrap(a) + Fixed18.unwrap(b));
}

function mul(Fixed6 a, Fixed6 b) pure returns (Fixed6) {
return Fixed6.wrap(Fixed6.unwrap(a) * Fixed6.unwrap(b) / Fixed6Lib.BASE);
}

function eq(Fixed6 a, Fixed6 b) pure returns (bool) {
return Fixed6.unwrap(a) == Fixed6.unwrap(b);
}

function gt(UFixed6 a, UFixed6 b) pure returns (bool) {
(uint256 au, uint256 bu) = (UFixed6.unwrap(a), UFixed6.unwrap(b));
return au > bu;
}

function neq(Fixed6 a, Fixed6 b) pure returns (bool) {
return Fixed6.unwrap(a) != Fixed6.unwrap(b);
}

function mul(Fixed18 a, Fixed18 b) pure returns (Fixed18) {
return Fixed18.wrap(Fixed18.unwrap(a) * Fixed18.unwrap(b) / Fixed18Lib.BASE);
}

function gt(Fixed6 a, Fixed6 b) pure returns (bool) {
(int256 au, int256 bu) = (Fixed6.unwrap(a), Fixed6.unwrap(b));
return au > bu;
}

function lt(UFixed18 a, UFixed18 b) pure returns (bool) {
(uint256 au, uint256 bu) = (UFixed18.unwrap(a), UFixed18.unwrap(b));
return au < bu;
}

function div(UFixed18 a, UFixed18 b) pure returns (UFixed18) {
return UFixed18.wrap(UFixed18.unwrap(a) * UFixed18Lib.BASE / UFixed18.unwrap(b));
}

function gte(UFixed6 a, UFixed6 b) pure returns (bool) {
return eq(a, b) || gt(a, b);
}

function neg(Fixed18 a) pure returns (Fixed18) {
return Fixed18.wrap(-Fixed18.unwrap(a));
}

function sub(Fixed6 a, Fixed6 b) pure returns (Fixed6) {
return Fixed6.wrap(Fixed6.unwrap(a) - Fixed6.unwrap(b));
}

function gte(Fixed6 a, Fixed6 b) pure returns (bool) {
return eq(a, b) || gt(a, b);
}

function mul(UFixed6 a, UFixed6 b) pure returns (UFixed6) {
return UFixed6.wrap(UFixed6.unwrap(a) * UFixed6.unwrap(b) / UFixed6Lib.BASE);
}

function lte(Fixed6 a, Fixed6 b) pure returns (bool) {
return eq(a, b) || lt(a, b);
}

function lte(UFixed18 a, UFixed18 b) pure returns (bool) {
return eq(a, b) || lt(a, b);
}

function mul(UFixed18 a, UFixed18 b) pure returns (UFixed18) {
return UFixed18.wrap(UFixed18.unwrap(a) * UFixed18.unwrap(b) / UFixed18Lib.BASE);
}

function lt(Fixed6 a, Fixed6 b) pure returns (bool) {
(int256 au, int256 bu) = (Fixed6.unwrap(a), Fixed6.unwrap(b));
return au < bu;
}

function div(Fixed6 a, Fixed6 b) pure returns (Fixed6) {
return Fixed6.wrap(Fixed6.unwrap(a) * Fixed6Lib.BASE / Fixed6.unwrap(b));
}

function neq(UFixed18 a, UFixed18 b) pure returns (bool) {
return UFixed18.unwrap(a) != UFixed18.unwrap(b);
}

function _deployer() internal view override returns (address) {
return IMutator(msg.sender).owner();
}

function div(Fixed18 a, Fixed18 b) pure returns (Fixed18) {
return Fixed18.wrap(Fixed18.unwrap(a) * Fixed18Lib.BASE / Fixed18.unwrap(b));
}

function lt(UFixed6 a, UFixed6 b) pure returns (bool) {
(uint256 au, uint256 bu) = (UFixed6.unwrap(a), UFixed6.unwrap(b));
return au < bu;
}

function lt(Fixed18 a, Fixed18 b) pure returns (bool) {
(int256 au, int256 bu) = (Fixed18.unwrap(a), Fixed18.unwrap(b));
return au < bu;
}

function gt(Fixed18 a, Fixed18 b) pure returns (bool) {
(int256 au, int256 bu) = (Fixed18.unwrap(a), Fixed18.unwrap(b));
return au > bu;
}

function lte(Fixed18 a, Fixed18 b) pure returns (bool) {
return eq(a, b) || lt(a, b);
}

function eq(Fixed18 a, Fixed18 b) pure returns (bool) {
return Fixed18.unwrap(a) == Fixed18.unwrap(b);
}

function sub(UFixed18 a, UFixed18 b) pure returns (UFixed18) {
return UFixed18.wrap(UFixed18.unwrap(a) - UFixed18.unwrap(b));
}

function neq(UFixed6 a, UFixed6 b) pure returns (bool) {
return UFixed6.unwrap(a) != UFixed6.unwrap(b);
}

function gt(UFixed18 a, UFixed18 b) pure returns (bool) {
(uint256 au, uint256 bu) = (UFixed18.unwrap(a), UFixed18.unwrap(b));
return au > bu;
}

function gte(UFixed18 a, UFixed18 b) pure returns (bool) {
return eq(a, b) || gt(a, b);
}

function sub(UFixed6 a, UFixed6 b) pure returns (UFixed6) {
return UFixed6.wrap(UFixed6.unwrap(a) - UFixed6.unwrap(b));
}

function add(UFixed6 a, UFixed6 b) pure returns (UFixed6) {
return UFixed6.wrap(UFixed6.unwrap(a) + UFixed6.unwrap(b));
}

function sub(Fixed18 a, Fixed18 b) pure returns (Fixed18) {
return Fixed18.wrap(Fixed18.unwrap(a) - Fixed18.unwrap(b));
}

function gte(Fixed18 a, Fixed18 b) pure returns (bool) {
return eq(a, b) || gt(a, b);
}

function lte(UFixed6 a, UFixed6 b) pure returns (bool) {
return eq(a, b) || lt(a, b);
}

function div(UFixed6 a, UFixed6 b) pure returns (UFixed6) {
return UFixed6.wrap(UFixed6.unwrap(a) * UFixed6Lib.BASE / UFixed6.unwrap(b));
}

function add(UFixed18 a, UFixed18 b) pure returns (UFixed18) {
return UFixed18.wrap(UFixed18.unwrap(a) + UFixed18.unwrap(b));
}

function neq(Fixed18 a, Fixed18 b) pure returns (bool) {
return Fixed18.unwrap(a) != Fixed18.unwrap(b);
}

solc-version

Impact: Informational
Confidence: High

pragma solidity ^0.8.19;

pragma solidity ^0.8.13;

pragma solidity >=0.8.20;

incorrect-using-for

Impact: Informational
Confidence: High

  • ID-68
    using-for statement at src/mutability/types/Version.sol#5 is incorrect - no matching function for Version found in VersionLib.

library VersionLib {
function from(uint32 major, uint32 minor, uint32 patch) internal pure returns (Version) {
return Version.wrap(bytes32((uint256(major) << 64) | (uint256(minor) << 32) | uint256(patch)));
}
}

missing-inheritance

Impact: Informational
Confidence: High

contract OwnableStub {
/// @notice Accepts ownership of the contract
/// @dev Can only be called by the pending owner to ensure correctness.
function acceptOwner(address ownable) external {
Ownable(ownable).acceptOwner();
}
}

https://github.com/equilibria-xyz/root/blob/9955006221ec660d8ef3a85f6503e0757ad17a45/src/mutability/Mutable.sol#L132-L137

naming-convention

Impact: Informational
Confidence: High

function __Pausable__constructor() internal initializer("Pausable") {
_updatePauser(_deployer());
}

function Mutable$() private pure returns (MutableStorage storage $) {
assembly {
$.slot := MutableStorageLocation
}
}

bytes32 private constant MutableStorageLocation = 0xb906736fa3fc696e6c19a856e0f8737e348fda5c7f33a32db99da3b92f19a800;

bytes32 private constant OwnableStorageLocation = 0x863176706c9b4c9b393005d0714f55de5425abea2a0b5dfac67fac0c9e2ffe00;

function __Ownable__constructor() internal initializer("Ownable") {
_updateOwner(_deployer());
}

bytes32 private constant ImplementationStorageLocation = 0x3c57b102c533ff058ebe9a7c745178ce4174563553bb3edde7874874c532c200;

function Ownable$() private pure returns (OwnableStorage storage $) {
assembly {
$.slot := OwnableStorageLocation
}
}

function Implementation$() private pure returns (ImplementationStorage storage $) {
assembly {
$.slot := ImplementationStorageLocation
}
}

bytes32 private constant PausableStorageLocation = 0x3f6e81f1674f7eaca7e8904fa6f14f10175d4d641e37fc18a3df849e00101900;

function Pausable$() private pure returns (PausableStorage storage $) {
assembly {
$.slot := PausableStorageLocation
}
}

function __constructor(bytes memory data) internal virtual returns (Version);

function Attribute$() private pure returns (AttributeStorage storage $) {
assembly {
$.slot := AttributeStorageLocation
}
}

bytes32 private constant AttributeStorageLocation = 0x429797e2de2710eed6bc286312ff2c2286e5c3e13ca14d38e450727a132bfa00;

unimplemented-functions

Impact: Informational
Confidence: High

abstract contract OwnerExecutable is Ownable {
/// @notice Executes a call to a target contract
/// @dev Can only be called by the owner
/// @param target Address of the target contract
/// @param data Calldata to be executed
/// @return result The result of the call
function execute(address target, bytes calldata data) public payable virtual onlyOwner returns (bytes memory) {
return Address.functionCallWithValue(target, data, msg.value);
}
}

abstract contract OwnerWithdrawable is Ownable {
/// @notice Withdraws all ERC20 tokens from the contract to the owner
/// @dev Can only be called by the owner
/// @param token Address of the ERC20 token
function withdraw(Token18 token) public virtual onlyOwner {
token.push(owner());
}
}

abstract contract OwnerDelegatable is Ownable {
/// @notice Delegates voting power for a specific token to an address
/// @dev Can only be called by the owner
/// @param token The IVotes-compatible token to delegate
/// @param delegatee The address to delegate voting power to
function delegate(IVotes token, address delegatee) public virtual onlyOwner {
token.delegate(delegatee);
}
}

abstract contract Implementation is IImplementation, Contract {
/// @custom:storage-location erc7201:equilibria.root.Implementation
struct ImplementationStorage {
bool constructing;
}
/// @dev The erc7201 storage location of the mix-in
// solhint-disable-next-line const-name-snakecase
bytes32 private constant ImplementationStorageLocation = 0x3c57b102c533ff058ebe9a7c745178ce4174563553bb3edde7874874c532c200;
/// @dev The erc7201 storage of the mix-in
function Implementation$() private pure returns (ImplementationStorage storage $) {
assembly {
$.slot := ImplementationStorageLocation
}
}
/// @dev The version of this implementation.
Version public immutable version;
/// @dev The version of the predecessor implementation.
Version public immutable predecessor;
/// @dev Constructor for the implementation.
constructor(Version version_, Version predecessor_) {
version = version_;
predecessor = predecessor_;
}
/// @dev The name of the implementation.
function name() external view virtual returns (string memory);
/// @dev Called at upgrade time to initialize the contract with `data`.
function construct(bytes memory data) external {
Implementation$().constructing = true;
Version constructorVersion = __constructor(data);
if (constructorVersion != version) revert ImplementationConstructorVersionMismatch();
Implementation$().constructing = false;
}
/// @dev Whether the contract is initializing.
function _constructing() internal view override returns (bool) {
return Implementation$().constructing;
}
/// @dev The deployer of the contract.
function _deployer() internal view override returns (address) {
return IMutator(msg.sender).owner();
}
/// @dev Hook for inheriting contracts to construct the contract.
function __constructor(bytes memory data) internal virtual returns (Version);
}


@EdNoepel EdNoepel changed the title Remove unused artifacts, fix coverage, Attribute tests Remove unused artifacts, fix coverage May 22, 2025
@EdNoepel EdNoepel marked this pull request as ready for review May 22, 2025 20:25
@EdNoepel EdNoepel requested review from kbrizzle and prateekdefi May 22, 2025 20:26
function _pause() internal {
for (uint256 i = 0; i < _mutables.length(); i++) IMutable(_mutables.at(i)).pause();
super._pause();
emit IMutableTransparent.Paused();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can forgo this event, the IMutableTransparent's should be emmitting their own version of these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed; removed in 227e58d.

}

function _pause() internal override {
function _pause() internal {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this was a problem before, but what happens if we (1) pause, then (2) create a new mutable, then (3) unpause.

I think step (3) would revert since the new mutable would not be in then "unpaused" state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree; two options:

  • Update Mutator.create to check the state of the first Mutable in the array (if any). Pause after creation if appropriate. This should ensure consistent state.
  • Update Mutator.pause and unpause to check state in each iteration. This seems safer but the cost would exascerbate the unbounded iteration.

Copy link
Contributor

@kbrizzle kbrizzle May 23, 2025

Choose a reason for hiding this comment

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

Yeah, I kind of don't love either option because it would also require us to put a getter in the fallback of Mutable which we don't have structure for already.

I'm wondering if it'd be easier to just not delete Pausable (actually forgot it was being used here), and instead add a fix to the create() function to pause the new Mutable on creation if Mutator is paused. While we don't need pausing functionality in Implementation-based contracts, we may want it still in Derived-based contracts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that. Reverted the change in 1d558a6 and implemented pause-on-create in 9f1f9a6.

@github-actions
Copy link

Unit Test Coverage Report

Coverage after merging ed/pe-2585 into v3 will be
100.00%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/attribute
   Attribute.sol100%100%100%100%
   Ownable.sol100%100%100%100%
   OwnableStub.sol100%100%100%100%
   OwnerDelegatable.sol100%100%100%100%
   OwnerExecutable.sol100%100%100%100%
   OwnerWithdrawable.sol100%100%100%100%
   Pausable.sol100%100%100%100%
src/distribution
   Airdrop.sol100%100%100%100%
   Treasury.sol100%100%100%100%
src/mutability
   Derived.sol100%100%100%100%
   Implementation.sol100%100%100%100%
   Mutable.sol100%100%100%100%
   Mutator.sol100%100%100%100%
src/mutability/types
   Version.sol100%100%100%100%
src/number
   NumberMath.sol100%100%100%100%
src/number/types
   Fixed18.sol100%100%100%100%
   Fixed6.sol100%100%100%100%
   UFixed18.sol100%100%100%100%
   UFixed6.sol100%100%100%100%
src/token/types
   Token.sol100%100%100%100%
   Token18.sol100%100%100%100%
   Token6.sol100%100%100%100%
src/utils
   console.sol100%100%100%100%
src/vrgda
   VRGDADecayMath.sol100%100%100%100%
   VRGDAIssuanceMath.sol100%100%100%100%
src/vrgda/types
   LinearExponentialVRGDA.sol100%100%100%100%

@EdNoepel EdNoepel requested a review from kbrizzle May 23, 2025 18:44
Copy link
Contributor

@prateekdefi prateekdefi left a comment

Choose a reason for hiding this comment

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

LGTM

@EdNoepel EdNoepel merged commit 188b332 into v3 May 26, 2025
3 checks passed
@EdNoepel EdNoepel deleted the ed/pe-2585 branch May 26, 2025 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants