code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/core/Core.sol#L20-L24

Vulnerability details

Impact

This is a standard front-runnable initializer vulnerability, but this instance in particular is very severe due to the permissions granted to the caller of init() in core.sol. See code below:

function init() external initializer {
        volt = new Volt(address(this));
        /// msg.sender already has the VOLT Minting abilities, so grant them governor as well
        _setupGovernor(msg.sender);
    }

The init() function instantiates VOLT and then grants the governor role to msg.sender. The comment above indicates that msg.sender will already have minting abilities, but if not, they would be able to call the grantMinter() function due to their governor permissions.

function grantMinter(address minter) external override onlyGovernor {
        grantRole(MINTER_ROLE, minter);
    }

Reviewing the deployCore.sh, it appears that core.sol is deployed first, followed by GlobalRateLimitedMinter.sol. The scripts attempts to call init() after deploying core, which will fail if an attacker calls it first. Eventually, when the script tries to call grantMinter() for GlobalRateLimitedMinter it will fail due to lack of governor permissions.

The key to the attack is that the attacker must call grantMinter() for GlobalRateLimitedMinter following the deployment of this contract. This will cause the deploy script to assume that the contracts were deployed successfully, as the only check is that isMinter(GlobalRateLimitedMinter_Address) == true .

IS_MINTER=$(cast call $CORE "isMinter(address) external view returns (bool)" $GLOBAL_RATE_LIMITED_MINTER)

if [ "$IS_MINTER" == "true" ]; then
    echo " ~~~ Successfully Deployed Contracts ~~~ "
    echo ""
    echo "CORE=$CORE"
    echo "GLOBAL_RATE_LIMITED_MINTER=$GLOBAL_RATE_LIMITED_MINTER"
    echo ""
else
    echo "Contracts Not Deployed Successfully"

Proof of Concept

Steps of attack:

Volt team deploys core.sol
Attacker calls init() in core.sol, receives governor role.
Volt team deploys GlobalRateLimitedMinter.sol
Attacker calls grantMinter() for GlobalRateLimitedMinter in core.sol, imported from permissions.sol
deployCore.sh does not detect that anything went wrong with the deployment due to GlobalRateLimitedMinter minter role == true.
Attacker has minter and governor abilities, unknown to team.

Tools Used

Manual review.

Recommended Mitigation Steps

Add access control to init(). Adding an onlyOwner modifier would eliminate this risk as it seems like the intent is to call init() within the same deploy script as deploying core.sol. Having init() without access control doesn't seem to add any desired functionality.

ElliotFriedman commented 2 years ago

contract can always be redeployed if another address or user init's the contract.

ElliotFriedman commented 2 years ago

This is a low severity issue

jack-the-pug commented 2 years ago

I'm pretty sure deployCore.sh#L17 will fail and return error, even if the attacker managed to send the init() before the script. I will downgrade this to QA.

code-423n4 / 2022-03-volt-findings