code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L94-L98

Vulnerability details

Impact

The purpose of a Timelock contract is to put a limit on the privileges of the governor, by forcing a two step process with a preset delay time.

However, we found that the current implementation actually won’t serve that purpose as it allows the onlyByOwnGov to execute any transactions without any constraints.

To do that, the current onlyByOwnGov can call setTimelock(address _timelock_address) and set a new timelock Address effective immediately.

In conclusion, a Timelock contract is supposed to guard the protocol from lost private key or malicious actions. The current implementation won’t fulfill that mission.

Proof of Concept

    function setTimelock(address _timelock_address) public onlyByOwnGov {
        require(_timelock_address != address(0), "Zero address detected"); 
        timelock_address = _timelock_address;
        emit TimelockChanged(_timelock_address);
    }

A similar vulnerability has been qualified as High; https://github.com/code-423n4/2021-11-malt-findings/issues/262

Tools Used

Manuel Code Review

Recommended Mitigation Steps

Consider making setTimelock function only callable from the Timelock contract itself.

Mitigation can be achieved by ensuring that all operations run under a time delay

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. If moving funds, it is assumed someone in the multisig would catch an invalid or malicious address.

0xean commented 2 years ago

dupe of #107

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