code-423n4 / 2022-02-jpyc-findings

1 stars 0 forks source link

Configureminer is vulnerable Double spends #46

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v2/FiatTokenV2.sol#L337-L347 https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/FiatTokenV1.sol#L327-L337

Vulnerability details

Configureminer is vulnerable Double spend

The function configureMiner at FiatTokenV1 and FiatTokenV2 is vulnerable to double spend the same as approve() of EIP20

function configureMinter(address minter, uint256 minterAllowedAmount)
    external
    whenNotPaused
    onlyMasterMinter
    returns (bool)
{
    minters[minter] = true;
    minterAllowed[minter] = minterAllowedAmount;
    emit MinterConfigured(minter, minterAllowedAmount);
    return true;
}

As you put in your dev comments you plan to use this function to update minterAllowed. The problem with that is that at a specific time the minterA (with A tokens allowed) is being updated to B tokens allowed. It can mint the A tokens before the transaction is mined and then get B extra tokens so being able to mint A+B.

Maybe you can consider implementing increaseMinerAllowance() and decreaseMinerAllowance or other similar solution.

retocrooman commented 2 years ago

Dupulicate of (#5) I don't do much approve other than contract, so I think it's Low Risk.

jack-the-pug commented 2 years ago

The precondition is the minter being malicious, plus, the impact is low. The warden's recommendation is not too bad.

I'm making this a low.

CloudEllie commented 2 years ago

Grouping this with warden's QA report, #44