tofunmi - DOS BY FRONTRUNNING `TimeLockNonTransferablePool` INITIALIZE() FUNCTION

[sherlock-admin]

tofunmi

medium

DOS BY FRONTRUNNING TimeLockNonTransferablePool INITIALIZE() FUNCTION

Summary

No check on who can call the initialize function in TimeLockNonTransferablePool

Impact

• DOS through front running.

Vulnerability Detail

Some of the contracts deployed utilize the Transparent upgradeable proxy standard. This standard involves first deploying an implementation contract and later a proxy contract which uses the implementation contract as its logic.

When users make calls to the proxy contract, the proxy contract will delegate call to the underlying implementation contract. TimeLockNonTransferablePool implement an initialize() function which aims to replace the role of the constructor() when deploying proxy contracts. It is important that these proxy contracts are deployed and initialized in the same transaction to avoid any malicious front-running. However, also checking the hardhat deployments style, they do not follow this pattern when deploying the Transparent proxy contract. As a result, a malicious attacker could monitor the Ethereum blockchain for bytecode that matches the TimeLockNonTransferablePool contract and front-run the initialize() transaction to gain ownership of the contract. This can be repeated as a Denial Of Service (DOS) type of attack, effectively preventing Merit's contract deployment, leading to unrecoverable gas expenses. This is very possible, cause the government proposals are public, so it is actually known that merit is deploying a v2 contract. The attacker just uses little gas to call call initialize.

Code Snippet

• https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockNonTransferablePool.sol#L7-L20

Tool used

Manual Review

Recommendation

• only the deployer should be able to initialize the contracts , a modifier check would solve this, to make sure only the proxy admin can call initialize, or having a constructor that sets the proxy admin in the implementation contract itself

contract TimeLockNonTransferablePool is TimeLockPool {
    address internal owner;
    constructor() {
        owner = msg.sender;
    }

    function initialize(
        string memory _name,
        string memory _symbol,
        address _depositToken,
        address _rewardToken,
        address _escrowPool,
        uint256 _escrowPortion,
        uint256 _escrowDuration,
        uint256 _maxBonus,
        uint256 _maxLockDuration,
        uint256[] memory _curve
    ) public initializer {
        __TimeLockPool_init(_name, _symbol, _depositToken, _rewardToken, _escrowPool, _escrowPortion, _escrowDuration, _maxBonus, _maxLockDuration, _curve);
        if (owner != msg.sender) revert();
    }

    // disable transfers
    function _transfer(address _from, address _to, uint256 _amount) internal override {
        revert("NON_TRANSFERABLE");
    }
}