Solv Protocol security record

The Solv Vault Guardian is designed to regulate multi-signature operations through a guard mechanism within the Safe. It utilizes the `checkTransaction()` function to validate the legitimacy of operations against configured rules for each `BaseAuthorization`. An operation proceeds if it meets at least one rule. The implementation allows for independent checks across multiple authorization contracts, with the process halting upon the first successful authorization. This design can inadvertently facilitate the execution of operations, such as token transfers and approvals if just one among potentially multiple authorization checks is lenient or explicitly permits the action.

The current test suite for the Solv Vault Guardian lacks comprehensive coverage potentially leaving the protocol vulnerable to untested edge cases and integration issues. Specifically, the testsuite does not fully address protocol compatibility, function selector accuracy, and various transaction scenarios, including those involving native currency and tokens across different protocols such as Agni, GMXV1, GMXV2, and Lendle. Identified Gaps: 1. **Lack of Protocol Compatibility Tests**: Lacks tests verifying Agni, GMXV1, GMX2, Lendle, and Cobo Argus protocol integrations. 2. **Lack of Safe Multisend Compatibility Tests**: Lack of tests verifying the guardian's validation of safe multisend functions. 3. **Safe Upgrade to 1.4**: No tests covering the upgrade of a Safe from version 1.3 to 1.4, missing checks for compatibility or security issues. 4. **Gnosis Safe Deployment in Tests**: The test suite relies on a deployed Safe instance, requiring access to an owner's private key, which complicates and potentially compromises testing integrity.

Upon deployment, both `GMXV1Authorization` and `GMXV2Authorization` add support for the ERC20 transfer and approve functions on a select number of whitelisted tokens. As a result, the Safe can freely invoke any transfer or approve operations on tokens held by the Safe, and bypass any of the necessary logic included in the `ERC20ApproveAuthorization` or `ERC20TransferAuthorization` contracts. Further, only the `GMXV1Authorization` would need to be enabled for there to be transfers, or approvals, of any whitelisted tokens from the SafeAccount. The overlap present can lead to an inconsistent state of approval, due to the fact that one authorizer would reject the transaction, whilst the other would allow it.

Upon detailed review of the `burn()` function, it is discovered that it lacks proper validation checks for scenarios where the input `sftId_` corresponds to the `holdingValueSftId` managed by the contract itself. In such cases, users engage the `burn()` function, expecting to exchange their ERC-20 tokens for an equivalent amount of ERC-3525 tokens. However, due to the absence of a check verifying that `sftId_` is distinct from `holdingSftId` and ensuring the caller is indeed the owner of `sftId_`, the ERC-20 tokens are burned without the corresponding release of ERC-3525 tokens to the user. This constitutes a critical flaw as it leads to a unilateral loss for the user without the anticipated token exchange.

The `swtAddress_` parameter of `unstake()` function is missing a check to ensure it is deployed by the `SftWrappedTokenFactory`, allowing users to specify an arbitrary address as the `swtAddress_`. Calling a user provided address is not safe, especially in a public function with no access control restriction. For example, if a non-standard ERC3525 token is transferred to the router without triggering `onERC3525Received` or `onERC721Received`, a malicious `swtAddress_` contract could be used to transfer the token to the attacker (L149).

Both `ERC20ApproveAuthroization` and `ERC3525ApproveAuthorization` are intended to enforce a whitelist of approved spenders for tokens held by the Safe. However, the lack of verification on the function selector of the transaction being invoked creates the possibility for an arbitrary transaction to be approved. As there is no ACL contract associated with these Authorizations, there is no `preCheck()` call that would fail in the case where a function is trying to be invoked that is not defined by the ACL. As a result, arbitrary functions can be more readily enabled and executed, as they only have to be added as an allowed selector to the Authorization. The exploitability of this issue is significantly limited by the fact that the Governor address would have to be compromised to support a malicious action, as well as the fact that for the malicious action to be invoked in the first place, there would still have to be the quorum of required signatures met on the Safe. Regardless, the possiblity of such should not be included in the Guard. The following exploit scenario considers the situation where the Governor is trying to transfer all USDT from the Safe, as we think this example is easy to follow. However, it could also work with any function on any contract that the compromised Governor account chooses to add. Although, the function called would still have to have arguments that can be decoded to `(address, uint256)`.

This [EIP](https://eips.ethereum.org/EIPS/eip-3525#erc-3525-token-receiver) defines a simple ‘Check, Notify and Response’ model for better flexibility as well as simplicity: 1. No extra `safeTransferFrom` methods are needed, all callers only need to call one kind of transfer; 2. All ERC-3525 contracts MUST check for the existence of `onERC3525Received` on the recipient contract and call the function when it exists; 3. Any smart contract can implement `onERC3525Received` function for the purpose of being notified after receiving values; this function MUST return 0x009ce20b (i.e. `bytes4(keccak256('onERC3525Received(address,uint256,uint256,uint256,bytes)'))`) if the transfer is accepted, or any other value if the transfer is rejected. There is a special case for this notification/acceptance mechanism: since ERC-3525 allows value transfer from an address to itself, when a smart contract which implements `onERC3525Received` transfers value to itself, `onERC3525Received` will also be called. This allows for the contract to implement different rules of acceptance between self-value-transfer and receiving value from other addresses. However, this contract does not follow the above rule and does not implement the `onERC3525Received()`.

When `acceptAdmin()` is called, the `pendingAdmin` variable is deleted without emitting an event that reflects its change to the zero address. While a `NewAdmin` event is emitted for the admin change, the lack of an explicit event for the `pendingAdmin` update can lead to inconsistencies for off-chain indexers or monitoring systems that rely on events to track the lifecycle of `pendingAdmin`.

The `withdrawFeeRate` parameter has no explicit upper bound enforced at the contract level. Although the calculation includes a safety check: `require(withdrawFee <= solvBTCAmount_)` this still allows configurations where the effective fee can approach 100% of the redemption amount, effectively resulting in near-zero `solvBTC` output for users, minimizing withdrawals amount for all users.

The report gives no description here.