code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L191-L196

Vulnerability details

Impact

The Timelock Address role is misidentified in this agreement and has high authority.

While I believe developer have good intention to use these functions. It often associate with Rug Pull by developer in the eyes of investors because Rug Pull is not uncommon in Defi.

Such definitions beyond the purpose of the Timelock Agreement may cause investors to suffer from trust issues.

Proof of Concept


    modifier onlyByOwnGov() {
        require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
        _;
    }

 function recoverEther(uint256 amount) external onlyByOwnGov {
        (bool success,) = address(owner).call{ value: amount }("");
        require(success, "Invalid transfer");

        emit EmergencyEtherRecovered(amount);
    }

Tools Used

Manuel Review

Recommended Mitigation Steps

1-Pause the Contract and Disable All Functions when Bad Thing Happen rather than Withdraw All Fund to a random address.

2-If Withdraw Fund can't avoid, a Multi Sig ETH Address should be hardcoded into the contract to ensure the fund move to a safe wallet.

FortisFortuna commented 2 years ago

We are well aware of the permission structure. The owner will most likely be a large multisig. We mentioned the Frax Multisig in the scope too.

joestakey commented 2 years ago

Duplicate of #107

code-423n4 / 2022-09-frax-findings