code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/LiquidityReserve.sol#L92 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L167-L170 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L217-L220 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L226-L229 https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L235-L238

Vulnerability details

Impact

The owner of the Yieldy contracts (according to the docs this is the ShapeShift DAO's multisig) can change critical parameters such as the liquidity reserve fee, epoch durations, warm-up periods, etc.

None of these setter functions emit events to record these changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary.

Neither of those setter functions uses timelocks to allow users to react in a timely manner.

Proof of Concept

LiquidityReserve.setFee

function setFee(uint256 _fee) external onlyOwner {
    // check range before setting fee
    require(_fee <= BASIS_POINTS, "Out of range");
    fee = _fee;

    emit FeeChanged(_fee);
}

Staking.setAffiliateFee

function setAffiliateFee(uint256 _affiliateFee) external onlyOwner {
        affiliateFee = _affiliateFee;
        emit LogSetAffiliateFee(block.number, _affiliateFee);
    }

Staking.setEpochDuration

function setEpochDuration(uint256 duration) external onlyOwner {
        epoch.duration = duration;
        emit LogSetEpochDuration(block.number, duration);
    }

Staking.setWarmUpPeriod

function setWarmUpPeriod(uint256 _vestingPeriod) external onlyOwner {
        warmUpPeriod = _vestingPeriod;
        emit LogSetWarmUpPeriod(block.number, _vestingPeriod);
    }

Staking.setCoolDownPeriod

    function setCoolDownPeriod(uint256 _vestingPeriod) external onlyOwner {
        coolDownPeriod = _vestingPeriod;
        emit LogSetCoolDownPeriod(block.number, _vestingPeriod);
    }

Tools Used

Manual review

Recommended mitigation steps

Add timelocks to introduce time delays for critical parameter changes.

toshiSat commented 2 years ago

disagree with severity: In an ideal world these will stay the same. If we want to change them it's due to needing to update the values immediately

JustDravee commented 2 years ago