BASE - kaveyjoe's results

Target : https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/universal/Proxy.sol

Summary: proxy contract that allows transparent proxy functionality. However, there are a few potential issues and areas that could be improved.

Bug 1: In the fallback() and receive() functions, there is a comment saying "Proxy call by default." However, there is no actual logic to handle the proxy call. This can lead to unexpected behavior and potentially cause the contract to behave incorrectly. It is recommended to remove the comment or implement the necessary logic for proxy call handling.

Bug 2: The _doProxyCall() function does not properly handle the case when the delegatecall to the implementation contract fails. Currently, it reverts with the error message "Proxy: implementation not initialized." However, this error message is misleading and does not accurately reflect the actual issue. It would be better to provide a more informative error message that indicates the delegatecall failure.

Improvement 1: The visibility of the upgradeTo() and _setImplementation() functions is set to public virtual. It is generally a good practice to make functions that modify internal state (_setImplementation() in this case) internal, rather than public. This helps prevent unintended access from external contracts and reduces the attack surface of the contract.

Improvement 2: The visibility of the changeAdmin() and _changeAdmin() functions is set to public virtual. Similar to Improvement 1, it is recommended to make these functions internal unless there is a specific reason for them to be public. This helps enforce proper access control and prevents unauthorized changes to the contract's admin.

Improvement 3: The admin() and implementation() functions both have the proxyCallIfNotAdmin modifier, which checks if the caller is the admin or address(0). However, it is not necessary to perform this check for read-only functions that do not modify the contract's state. It is recommended to remove the proxyCallIfNotAdmin modifier from these functions to improve gas efficiency.

Other Recommendations:

Add explicit visibility specifiers (e.g., internal, public, external) to function declarations for better readability and to make the code more explicit. Consider adding additional error handling and validation to ensure that only valid addresses are used for admin and implementation changes. Add appropriate access control checks, such as a onlyAdmin modifier, to functions that modify the contract's state.

Targt : https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/legacy/L1ChugSplashProxy.sol

Lack of Function Access Control: The contract does not enforce access control for critical functions such as upgradeTo, upgradeToAndCall, and changeAdmin. These functions should only be callable by the owner/admin of the contract. Without proper access control, anyone can potentially upgrade the implementation or change the admin, compromising the integrity and security of the system.

Use of Delegatecall: The _doProxyCall function utilizes the delegatecall opcode, which executes the code of the implementation contract within the context of the proxy contract. While this enables upgradability, it also introduces security risks. Malicious implementation contracts could exploit this behavior to manipulate the state of the proxy contract, leading to unauthorized access or manipulation of funds.

Potential Reentrancy Attacks: The contract is susceptible to reentrancy attacks when interacting with external contracts during the delegatecall operation. If the implementation contract calls back into the proxy contract before completing the execution, it can reenter the fallback or receive functions, potentially leading to unexpected behavior and unauthorized transfers of funds.

Lack of Input Validation: The contract does not perform sufficient input validation in the upgradeToAndCall function. Malformed or malicious _data could cause unexpected behavior or vulnerabilities in the implementation contract.

Gas Limit and Out-of-Gas Issues: The proxy contract forwards all available gas to the implementation contract during the delegatecall operation. If the implementation contract consumes excessive gas or enters an infinite loop, the transaction invoking the proxy contract may run out of gas and fail. Care should be taken to ensure that the implementation contract behaves efficiently and avoids gas-related issues.

Limited Ownership Transfer Security: The contract allows for changing the owner/admin address through the changeAdmin function. However, there are no additional security measures such as multi-signature requirements or time-based restrictions for ownership transfer. This could lead to unauthorized ownership changes and potential control over the proxy contract.