StrategyRegistry Manual Review Findings

SRY-01M: Inexistent Initialization of Base Implementation

Type	Severity	Location
Language Specific		StrategyRegistry.sol:L26-L34

Description:

The contract does not properly initialize the base logic implementation permitting it to be taken over by a malicious party.

Impact:

While not an active security threat, it can evolve into one if any form of delegatecall capability is introduced in one of the dependencies of the contract that could cause it to invoke a selfdestruct instruction.

Example:

Copy
26contract StrategyRegistry is
27    Initializable,
28    ERC721Upgradeable,
29    ERC721URIStorageUpgradeable,
30    ERC721EnumerableUpgradeable,
31    ERC721PausableUpgradeable,
32    AccessControlUpgradeable,
33    UUPSUpgradeable,
34    OwnableUpgradeable
35{

Recommendation:

We advise a constructor to be introduced that simply invokes the initializer modifier to ensure that the logic implementation cannot be initialized maliciously.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

A constructor was introduced that properly invokes the initializer modifier and disallows initialization of the logic implementation, alleviating this exhibit in full.

SRY-02M: Inexistent Sanitization of Max Gas

Type	Severity	Location
Input Sanitization		StrategyRegistry.sol:L315-L320

Description:

The maxMaxGasPerAction variable is not sanitized in the setMaxMaxGasPerAction function, thus permitting the registry owner to set it to an abnormally low value that would cause actions to be inexecutable.

Impact:

The owner of the contract could adjust the maxMaxGasPerAction to an abnormal value that would render future strategies inexecutable.

Example:

Copy
315function setMaxMaxGasPerAction(uint256 _maxMaxGasPerAction)
316    external
317    onlyOwner
318{
319    maxMaxGasPerAction = _maxMaxGasPerAction;
320}

Recommendation:

We advise a minimum lower bound to be imposed by the setMaxMaxGasPerAction function, potentially equal to the 15_000_000 default value set during the contract's initialize function.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

A minimum of 15_000_000 units is now enforced by the setMaxMaxGasPerAction function thereby alleviating this exhibit in full.

SRY-03M: Inexistent Validation of execBundle Validity

Type	Severity	Location
Logical Fault		StrategyRegistry.sol:L103

Description:

The createStrategy system assumes that an empty execBundle is an inexistent strategy entry, however, the createStrategy function does not prevent the execBundle of a new entry from being empty.

Impact:

It is currently possible to mint a strategy and completely redefine it after creation, ultimately containing multiple NFTs pointing to the same strategy and compromising the strategy system.

Example:

Copy
100function createStrategy(
101    address strategyCreator,
102    string calldata strategyName,
103    string calldata execBundle,
104    uint128 maxGasCost,
105    uint128 maxGasPerAction
106) external returns (uint256 newStrategyTokenId) {
107    // Check if the strategy is already registered
108    // This occurs when the bundle has the same CID as a previously registered bundle
109    require(
110        keccak256(abi.encodePacked(strategies[execBundle].execBundle)) ==
111            hashedEmptyString,
112        "Strategy already exists"
113    );
114
115    // Validate gas config
116    require(
117        maxGasPerAction <= maxMaxGasPerAction,
118        "maxGasPerAction too high"
119    );
120
121    // Mint a new token to the current sender
122    newStrategyTokenId = mint(strategyCreator, execBundle);

Recommendation:

We advise the execBundle variable to be properly sanitized as not-empty as otherwise strategies that have been minted could be overwritten in the registry.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

The createStrategy function was updated to disallow an execBundle hash being equal to the hash of an empty string. Additionally, the existence check was updated to instead evaluate whether the execBundle mapping entry contains a value different than the value of the hashed value instead of evaluating whether the entry is equal to the hash of an empty entry. As a result, we consider this exhibit adequately alleviated.

SRY-04M: Arbitrary Burn Functionality

Type	Severity	Location
Logical Fault		StrategyRegistry.sol:L264-L266

Description:

The burn function is incorrectly exposed with no access control permitting any user to burn arbitrary tokenId NFTs held by other parties.

Impact:

Strategies can be maliciously deleted by any member thus compromising the integrity of the system.

Example:

Copy
264function burn(uint256 tokenId) external {
265    _burn(tokenId);
266}

Recommendation:

Based on additional communication with the Steer Protocol team, we advise the function to be omitted from the codebase entirely as strategies are not expected to be removed from circulation over their lifetime.

Alleviation (200f275c40cbd4798f4a416c044ea726755d4741):

The burn function has been properly omitted from the codebase thus alleviating this exhibit.