sherlock-audit / 2024-02-telcoin-platform-audit-update-judging

3 stars 1 forks source link

bareli - Unclaimed or front-runnable proxy implementations #79

Closed sherlock-admin4 closed 8 months ago

sherlock-admin4 commented 8 months ago

bareli

medium

Unclaimed or front-runnable proxy implementations

Summary

Various smart contracts in the system require initialization functions to be called. The point when these calls happen is up to the deploying address. Deployment and initialization in one transaction are typically safe, but it can potentially be front-run if the initialization is done in a separate transaction.

A frontrunner can call these functions to silently take over the contracts and provide malicious parameters or plant a backdoor during the deployment.

Leaving proxy implementations uninitialized further aides potential phishing attacks where users might claim that - just because a contract address is listed in the official documentation/code-repo - a contract is a legitimate component of the system. At the same time, it is ‘only’ a proxy implementation that an attacker claimed. For the end-user, it might be hard to distinguish whether this contract is part of the system or was a maliciously appropriated implementation.

Vulnerability Detail

function initialize( address beacon, bytes memory data ) external initializer { ERC1967Utils.upgradeBeaconToAndCall(beacon, data); }

Impact

Code Snippet

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/external/openzeppelin/ClonableBeaconProxy.sol#L35

Tool used

Manual Review

Recommendation

It is recommended to use constructors wherever possible to immediately initialize proxy implementations during deploy-time. The code is only run when the implementation is deployed and affects the proxy initializations. If other initialization functions are used, we recommend enforcing deployer access restrictions or a standardized, top-level initialized boolean, set to true on the first deployment and used to prevent future initialization.

Using constructors and locked-down initialization functions will significantly reduce potential developer errors and the possibility of attackers re-initializing vital system components.

sherlock-admin3 commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

front-running initialization is invalid under sherlock's rules