Farseen0/scar-pairs huggingface dataset:
LOW severity: `platformFee` can be set above 1e18, which would disable all claims. The owner can set the platform fee at any time using this function:

There are no checks on this value. However, it is crucial that this value remain below `1e18` or the contract will no longer work, because in the `_claim()` function, when the `feeAmount` is calculated, we perform this...

Vulnerable code:
function setPlatformFee(uint256 _platformFee) external onlyOwner {
    fee = _platformFee;

    emit FeeUpdated(_platformFee);
}

if (fee != 0) {
    feeAmount = amount.mulWad(fee);
    amount -= feeAmount;
    feeAccrued[bounty.rewardToken] += feeAmount;
}
MEDIUM severity: Assertion that claimed <= burned should come after tokens have been claimed. In both the `claim()` and `buyBackFromL1()` functions, there is the following assertion:

This is used to ensure that the most important invariant of the system is always upheld. It is also used so that, if a user is able to trigger this state, an off chain guardian can pause the contract so the...

Vulnerable code:
assert(totalAmountClaimed <= totalAmountBurntOnL1);
MEDIUM severity: Possible reentrancy attack vector when buying back tokens from. When tokens are burned on L1 and claimed on L2, we always increment the `l1BurntAmountOf`. We then attempt to transfer the tokens to the user and — if it is successful — we increment `claimedAmountOf`.

This breaks the Checks-Effects-Interactions pattern commonly used to avoid reentrancy risk.

If.....

Vulnerable code:
...

l1BurntAmountOf[l1Depositor] = totalAmountBurntOnL1;

// The reason we are using try-catch block is that we want to store the `totalAmountBurntOnL1`
// regardless of the failure of the `_buyBack` function. This allows for the depositor
// to claim their share on L2 later.
try this._buyBack(receiver, burnTokenAmount) returns (
    uint256 buyTokenAmount
) {
    // Updating the amount claimed against the tokens burnt by the `l1Depositor`.
    claimedAmountOf[l1Depositor] += burnTokenAmount;
    ...
}
LOW severity: Being set as receiver during burn does not guarantee receiving tokens. When a user burns a token on L1 to receive it on L2, they call the following function:

The function takes in an amount of tokens to burn, as well as a receiver for the MTy tokens on L2.

This may seem to be guaranteeing that the `receiver` will be the address to receive the tokens on L2. We could.....

Vulnerable code:
function buyBack(
    address receiver,
    uint256 burnTokenAmount
) external whenNotPaused whenL2ComptrollerSet {
    ...
}

function claimAll(address receiver) external whenNotPaused {
    // The difference between burnt amount and previously claimed amount gives us
    // the claimable amount in `tokenToBurn` denomination.
    claim(
        receiver,
        l1BurntAmountOf[msg.sender] - claimedAmountOf[msg.sender]
    );
}
LOW severity: [G-01] whenNotPaused modifier can be removed from `claimAll()` to save gas. The `claimAll()` function contains the `whenNotPaused` modifier:

However, this function calls `claim()`, which already contains this modifier:

We can therefore remove the modifier from the `claimAll()` function to save a small amount of gas, while keeping the contract's behavior...

Vulnerable code:
function claimAll(address receiver) external whenNotPaused {
    // The difference between burnt amount and previously claimed amount gives us
    // the claimable amount in `tokenToBurn` denomination.
    claim(
        receiver,
        l1BurntAmountOf[msg.sender] - claimedAmountOf[msg.sender]
    );
}

function claim(
    address receiver,
    uint256 burnTokenAmount
) public whenNotPaused {
    ...
}

- function claimAll(address receiver) external whenNotPaused {
+ function claimAll(address receiver) external {
     // The difference between burnt amount and previously claimed amount gives us
     // the claimable amount in `tokenToBurn` denomination.
     claim(
         receiver,
         l1BurntAmountOf[msg.sender] - claimedAmountOf[msg.sender]
     );
 }
LOW severity: [I-02] When Comptroller is low on funds, large claims will fail while small claims will succeed. When a user claims tokens on L2 (whether through `buyBackFromL1()`, `buyBack()`, `claim()` or `claimAll()`), the tokens are transferred with the following logic:

It is expected that, at times, the contract will not have sufficient tokens to perform this transfer, as it will be reloaded manually...

Vulnerable code:
// Transfer the tokens to the caller.
// We are deliberately not checking if this contract has enough tokens as
// this would have the desired impact in case of low buy token balance anyway.
IERC20Upgradeable(address(tokenToBuy)).safeTransfer(
    receiver,
    buyTokenAmount
);

function _buyBack(
    address receiver,
    uint256 burnTokenAmount
) external returns (uint256 buyTokenAmount) {

    ...
    buyTokenAmount = (burnTokenAmount * exchangePrice) / tokenToBuyPrice;
+   uint contractBalance =  IERC20Upgradeable(address(tokenToBuy)).balanceOf(address(this));
+   buyTokenAmount = buyTokenAmount > contractBalance ? contractBalance : buyTokenAmount;

    // Transfer the tokens to the caller.
    // We are deliberately not checking if this contract has enough tokens as
    // this would have the desired impact in case of low buy token balance anyway.
    IERC20Upgradeable(address(tokenToBuy)).safeTransfer(
        receiver,
        buyTokenAmount
    );

    ...
HIGH severity: currentMultiplier is calculated incorrectly, causing up to 86400x discounted issuance. Multiplier.sol returns a `currentMultiplier` value, which is intended to represent the multiplier up to the current second (as opposed to the tracked value, which is rounded to the latest day). The value is calculated as follows:

To translate: If there is a delta between the end of the latest day.....

Vulnerable code:
uint256 dT = block.timestamp - trackedTimestamp;

if (dT != 0) {
    uint256 feePerSecondScaled = oneSubFee / 1 days;

    currentMultiplier = fmul(
        trackedMultiplier,
        feePerSecondScaled * dT
    );
} else {
    currentMultiplier = trackedMultiplier;
}

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "src/lib/Multiplier.sol";

contract ZachTest is Test {
    function testZ__CurrentMultiplierAt1Day() public {
        uint lastTs = block.timestamp;
        vm.warp(block.timestamp + 1 days);
        uint lastMul = 1e18;
        uint feeScaled = 27260808837036;
        (,,, uint currMul) = Multiplier.computeMultiplier(lastTs, lastMul, feeScaled);
        console.log(currMul);
    }

    function testZ__CurrentMultiplierAt1DayPlus1() public {
        uint lastTs = block.timestamp;
        vm.warp(block.timestamp + 1 days + 1);
        uint lastMul = 1e18;
        uint feeScaled = 27260808837036;
        (,,, uint currMul) = Multiplier.computeMultiplier(lastTs, lastMul, feeScaled);
        console.log(currMul);
    }
}

Running 2 tests for test/Zach.t.sol:ZachTest
[PASS] testZ__CurrentMultiplierAt1Day() (gas: 7212)
Logs:
  999972739191162964

[PASS] testZ__CurrentMultiplierAt1DayPlus1() (gas: 7627)
Logs:
  11573443045433
HIGH severity: Token accounting can be permanently broken by malicious rebalancer. The `vault` holds a `nominals` value (also known as a virtual balance) for each asset. This value represents the amount of each asset that the vault should hold per 1e18 AMKT tokens. It is an invariant of the system that `nominals[asset] * totalSupply / 1e18` should always equal the asset's...

Vulnerable code:
try Rebalancer(msg.sender).rebalanceCallback() {} catch {
    revert BountyCallbackFailed();
}

if (indexToken.totalSupply() != startingSupply) {
    revert BountyAMKTSupplyChange();
}
HIGH severity: If `feeScaled` is set too high in the constructor, the protocol will be bricked. When `feeScaled` is set using the `setFeeScaled()` function, we perform a check to ensure it is lower than `1e18`:

However, when the value is set from the constructor, there is no such check:

This means that it is possible for the constructor to set the value to be greater than `1e18`.

This is...

Vulnerable code:
function setFeeScaled(uint256 _feeScaled) external onlyOwner {
    if (_feeScaled > SCALAR) {
        revert AMKTVaultFeeTooLarge();
    }

    tryInflation();
    feeScaled = _feeScaled;
}

constructor(
    IIndexToken _indexToken,
    address _owner,
    address _feeRecipient,
    uint256 _feeScaled
) {
    indexToken = _indexToken;
    owner = _owner;

    feeRecipient = _feeRecipient;
    feeScaled = _feeScaled;

    lastKnownMultiplier = SCALAR;
    lastKnownTimestamp = block.timestamp;
}

function computeMultiplier(
        uint256 lastTrackedTimestamp,
        uint256 lastTrackedMultiplier,
        uint256 feeScaled
    )
        internal
        view
        returns (
            uint256 trackedTimestamp,
            uint256 trackedMultiplier,
            uint256 newFeeAccrued,
            uint256 currentMultiplier
        )
    {
        if (feeScaled > SCALAR) {
            revert MultiplierFeeTooHigh();
        }

        ...
}
HIGH severity: Fees are sent to vault instead of feeRecipient. Each time AMKT tokens are issued or redeemed, or a bounty is fulfilled, a call is made to `vault.tryInflation()`. This function checks if a day has passed since the last inflation and, if it has, takes protocol fees in the form of newly minted AMKT.

These protocol fees are intended to be paid to...

Vulnerable code:
function tryInflation() public returns (uint256) {
    uint256 startingSupply = indexToken.totalSupply();

    (
        uint256 timestamp,
        uint256 trackedMultiplier,
        uint256 newFeeAccrued,
        uint256 current
    ) = multiplier();

    uint256 inflation = fmul(startingSupply, finv(newFeeAccrued)) - startingSupply;

    if (inflation > 0) {
        lastKnownMultiplier = trackedMultiplier;
        lastKnownTimestamp = timestamp;
        indexToken.mint(address(this), inflation);
    }

    return current;
}

if (inflation > 0) {
    lastKnownMultiplier = trackedMultiplier;
    lastKnownTimestamp = timestamp;
-   indexToken.mint(address(this), inflation);
+   indexToken.mint(feeRecipient, inflation);
}
MEDIUM severity: First mint could be frontrun to break `totalSupply` calculation. In order to perform the migration for the AMKT token from L1 to L2, the following process is used:
- First, the `totalSupply` of the L1 token is minted on L2
- Then, it is sent to the L2 bridge, so that the full quantity is locked in the bridge ready to be withdrawn
- When the funds arrive to L1,...

Vulnerable code:
function mint(
    address _to,
    uint256 _amount
)
    external
    virtual
    override(IOptimismMintableERC20, ILegacyMintableERC20)
    onlyBridge
{
    // ignores the first mint
    if (MIGRATED) {
        _mint(_to, _amount);
        emit Mint(_to, _amount);
    } else {
        MIGRATED = true;
    }
}
MEDIUM severity: Owner can steal all funds locked in bridge by changing `REMOTE_TOKEN` value. In the new `BridgedIndexToken.sol`, which the L1 contract will be upgraded to, there is a `setBridge()` function used to set the `REMOTE_TOKEN` and the `BRIDGE` addresses.

This function can be called by the owner at any time to update these two addresses.

The addresses are each used in important.....

Vulnerable code:
function setBridge(
    address _remoteToken,
    address _bridge
) external onlyOwner {
    REMOTE_TOKEN = _remoteToken;
    BRIDGE = _bridge;
}

function setBridge(
    address _remoteToken,
    address _bridge
) external onlyOwner {
+   require(REMOTE_TOKEN == address(0) && BRIDGE == address(0), "values already set");
+   require(_remoteToken != address(0) && _bridge != address(0), "cannot set to zero addr");
    REMOTE_TOKEN = _remoteToken;
    BRIDGE = _bridge;
}
MEDIUM severity: `proposedOwner` not reset on transfer, allowing ownership to be seized back. The ProposableOwnable.sol contract amends OpenZeppelin's Ownable Upgradeable contract to add in a two step ownership transfer process, where a new owner must be proposed and then accepted.

Because this contract inherits all the functionality of `OwnableUpgradeable.sol`, it also allows ownership...

Vulnerable code:
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

/**
 * @dev Transfers ownership of the contract to a new account (`newOwner`).
 * Can only be called by the current owner.
 */
function transferOwnership(address newOwner) public virtual onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    _transferOwnership(newOwner);
}

/**
 * @dev Transfers ownership of the contract to a new account (`newOwner`).
 * Internal function without access restriction.
 */
function _transferOwnership(address newOwner) internal virtual {
    address oldOwner = _owner;
    _owner = newOwner;
    emit OwnershipTransferred(oldOwner, newOwner);
}

function takeOwnership(address newOwner) public virtual {
    require(
        newOwner != address(0),
        "ProposableOwnable: new owner is the zero address"
    );
    require(
        newOwner == proposedOwner,
        "ProposableOwnable: new owner is not proposed owner"
    );
    require(
        newOwner == msg.sender,
        "ProposableOwnable: this call must be made by the new owner"
    );
    _transferOwnership(newOwner);
}

function testZ__ProposedOwner() public {
    // Initialize a token that uses ProposableOwnable, setting myself to owner
    IndexToken token = new IndexToken();
    token.initialize("Fake Token", "FAKE", 1e18);
    console.log(token.owner());

    // Propose myself as the new owner.
    token.proposeOwner(address(this));

    // Renounce my ownership, apparently giving up control.
    token.renounceOwnership();
    console.log(token.owner());

    // Because I'm still in the proposedOwner slot, I can still reclaim.
    token.takeOwnership(address(this));
    console.log(token.owner());
}
MEDIUM severity: If bounty is created without all underlying tokens, accounting will be permanently thrown off. When a bounty is created, it includes a `nominal` value for each underlying token.

If the token is not going to be adjusted, the `nominal` value should equal the previous nominal amount, minus all fees that have accrued since then.

This is because, at the end of rebalancing, the `nominal` value...

Vulnerable code:
uint256 realUnitsAtLastFeeTimestamp = fmul(
    vault.virtualUnits(token),
    trackedMultipler
);

if (realUnitsAtLastFeeTimestamp > targetUnits) {
    uint256 diff = realUnitsAtLastFeeTimestamp - targetUnits;
    uint256 underlyingAmount = fmul(diff, startingSupply);
    ....

function invokeSetNominal(
    SetNominalArgs[] calldata args
) external onlyRebalancer {
    for (uint256 i; i < args.length; i++) {
        _setNominal(args[i]);
    }
}

function _setNominal(SetNominalArgs memory args) internal {
    address token = args.token;
    uint256 _virtualUnits = args.virtualUnits;

    if (_virtualUnits == 0) {
        delete nominals[token];
        _underlying.remove(token);
        return;
    }

    if (!isUnderlying(token)) {
        _underlying.add(token);
    }

    nominals[token] = _virtualUnits;
}
MEDIUM severity: Allowing multiple bounties to be live at once creates race condition. When the `authority` address sets up a bounty in `HashStore.sol`, they call `setHash()`, which adds the bounty hash to the `isValidHash` mapping:

While each bounty does have a deadline field (so old bounties are unlikely to linger for longer than intended), this architecture allows for multiple...

Vulnerable code:
function setHash(bytes32 _hash, bool isValid) external {
    if (msg.sender != authority) revert HashStoreAuth();
    isValidHash[_hash] = isValid;
}

bytes32 bountyHash = hashBounty(bounty);
if (bountyHash != hashStore.bountyHash) revert BountyInvalidHash();
MEDIUM severity: Inflation will be overapplied if there is a time period when `totalSupply() == 0`. When any action is taken in the protocol, `tryInflation()` is called, which checks the multiplier since the last known timestamp and, if there is inflation to apply, mints new AMKT tokens as a fee on the protocol.

As we can see, this function begins by calling `multiplier()`, which calls...

Vulnerable code:
function tryInflation() public returns (uint256) {
    uint256 startingSupply = indexToken.totalSupply();

    (
        uint256 timestamp,
        uint256 trackedMultiplier,
        uint256 newFeeAccrued,
        uint256 current
    ) = multiplier();

    uint256 inflation = fmul(startingSupply, finv(newFeeAccrued)) - startingSupply;

    if (inflation > 0) {
        lastKnownMultiplier = trackedMultiplier;
        lastKnownTimestamp = timestamp;

        indexToken.mint(address(this), inflation);
    }

function tryInflation() public returns (uint256) {
    uint256 startingSupply = indexToken.totalSupply();

    (
        uint256 timestamp,
        uint256 trackedMultiplier,
        uint256 newFeeAccrued,
        uint256 current
    ) = multiplier();

-   uint256 inflation = fmul(startingSupply, finv(newFeeAccrued)) - startingSupply;

-   if (inflation > 0) {
+   if (newFeeAccrued < SCALAR) {
       lastKnownMultiplier = trackedMultiplier;
       lastKnownTimestamp = timestamp;

+      uint256 inflation = fmul(startingSupply, finv(newFeeAccrued)) - startingSupply;
+      if (inflation > 0 {
           indexToken.mint(address(this), inflation);
+      }
    }

    return current;
}
MEDIUM severity: System implicitly relies on off chain oracle. While many smart contract systems rely on on-chain oracles to determine asset prices and calculations, the rebalancing calculations for the AMKT vault is performed off-chain by the Alongside team, and uploaded in the form of a hash to the HashStore:

There are trade offs to this methodology.

On...

Vulnerable code:
function setHash(bytes32 _hash, bool isValid) external {
    if (msg.sender != authority) revert HashStoreAuth();
    isValidHash[_hash] = isValid;
}
LOW severity: Issuer can underpay for AMKT because values are rounded down. When `issue()` is called to mint new `AMKT`, we calculate the `underlyingAmount` of each asset to be deposited by multiplying the real units (nominal units discounted for fees) by the amount of `AMKT` that is being minted.

All multiplication values in Solidity are rounded down. This means that...

Vulnerable code:
uint256 underlyingAmount = fmul(tokens[i].units, amount);

function issue(uint256 amount) external {
    vault.tryInflation();
    TokenInfo[] memory tokens = vault.realUnits(); // nominal[asset] * multiplier

    for (uint256 i; i < tokens.length; ) {
-       uint256 underlyingAmount = fmul(tokens[i].units, amount);
+       uint256 underlyingAmount = fmul(tokens[i].units, amount) + 1;
        IERC20(tokens[i].token).transferFrom(
            msg.sender,
            address(vault),
            underlyingAmount
        );

        unchecked {
            ++i;
        }
    }

    vault.invokeMint(msg.sender, amount);
}
HIGH severity: System cannot support atypical ERs. When tokens are transferred to or from the vault, the `transfer()` and `transferFrom()` functions are used.

Technically, the ERC20 spec dictates that tokens must return `false` in the event that a transfer fails, but it is not necessarily the case that the function will revert.

While that is the.....

Vulnerable code:
IERC20(tokens[i].token).transferFrom(
    msg.sender,
    address(vault),
    underlyingAmount
);

function _invokeERC20(address token, address to, uint256 amount) internal {
    IERC20(token).transfer(to, amount);
}
HIGH severity: Yield cannot be harvested because wrong token is passed to Incentives Controller. In order to harvest rewards from Silo, the SiloAMO contains a `harvestYield()` function. This function checks the rewards balance of the contract, and then calls `claimRewards()` to claim them.

Currently, the `harvestYield()` function is set to use the `OHM` token. However, Silo does not pay...

Vulnerable code:
function harvestYield() external {
    ISiloIncentivesController incentivesController_ = ISiloIncentivesController(
        incentivesController
    );

    address[] memory assets = new address[](1);
    assets[0] = address(OHM);

    // Get claimable rewards
    uint256 claimableRewards = incentivesController_.getRewardsBalance(assets, address(this));

    // Claim rewards
    if (claimableRewards > 0)
        incentivesController_.claimRewards(assets, claimableRewards, address(this));
}

contract SiloRewardsTest is Test {
    address ohm = 0x64aa3364F17a4D01c6f1751Fd97C2BD3D7e7f1D5;
    address collateralToken = 0x907136B74abA7D5978341eBA903544134A66B065;
    address collateralOnlyToken = 0xFDdfEd73b29B8859c6AE234aD64E2841614De559;
    address debtToken = 0x85A44Ff42F6B89125a541F64c77840977b0097E2;

    SiloIncentivesController incentives = SiloIncentivesController(0x6c1603aB6CecF89DD60C24530DdE23F97DA3C229);

    function testWhichTokensHaveIncentives() public {
        vm.createSelectFork("RPC_URL", 17407114);
        uint104 emissionsPerSecond;

        (emissionsPerSecond,,) = incentives.assets(ohm);
        console.log("OHM: ", emissionsPerSecond);

        (emissionsPerSecond,,) = incentives.assets(collateralToken);
        console.log("Collateral: ", emissionsPerSecond);

        (emissionsPerSecond,,) = incentives.assets(collateralOnlyToken);
        console.log("Collateral Only: ", emissionsPerSecond);

        (emissionsPerSecond,,) = incentives.assets(debtToken);
        console.log("Debt: ", emissionsPerSecond);
    }
}

Logs:
  OHM:  0
  Collateral:  6430041152263380
  Collateral Only:  0
  Debt:  38580246913580
MEDIUM severity: ohmDeployed will become inaccurate if circulatingOhmBurned is used. The OHM deposited to Silo will accure interest over time, which means that the protocol has the potential to withdraw more OHM than is tracked by the `ohmDeployed` value.

This is expected, and the Policy performs valid adjustments to account for it:

If more OHM is withdrawn than was deployed,...

Vulnerable code:
if (ohmDeployed < amount_) circulatingOhmBurned += amount_ - ohmDeployed;
ohmDeployed -= ohmDeployed > amount_ ? amount_ : ohmDeployed;

function _deposit(uint256 amount_) internal {
    // Update state
+   uint cachedCirculatingOhmBurned = circulatingOhmBurned;
+   if (cachedCirculatingOhmBurned > amount_) {
+       circulatingOhmBurned -= amount_;
+   else if (cachedCirculatingOhmBurn > 0) {
+       circulatingOhmBurned = 0;
+       ohmDeployed += amount_ - cachedCirculatingOhmBurned;
+   } else {
        ohmDeployed += amount_;
+   }
MEDIUM severity: SiloAMO uses outdated InterestRateModel. The new SiloAMO policy is intended to be deployed with the Silo interest rate model at address `0x7e9e7ea94e1ff36e216a703D6D66eCe356a5fd44` (as used in the fork tests and confirmed by the Olympus team).

If we check that contract address, we can see that the file deployed is...

Vulnerable code:
/// @notice DEPRECATED. Please use InterestRateModelV2 instead.

interface SiloRepository {
    function getInterestRateModel(address silo, address ohm) external view returns (IInterestRateModel);
}

interface IInterestRateModel {}

contract InterestRateModelTest is Test {
    SiloRepository rep = SiloRepository(0xd998C35B7900b344bbBe6555cc11576942Cf309d);

    function testCorrectInterestRateModel() public {
        vm.createSelectFork("https://mainnet.infura.io/v3/fb419f740b7e401bad5bec77d0d285a5");
        address silo = 0xf5ffabab8f9a6F4F6dE1f0dd6e0820f68657d7Db;
        address ohm = 0x64aa3364F17a4D01c6f1751Fd97C2BD3D7e7f1D5;
        address model = address(rep.getInterestRateModel(silo, ohm));
        console.log(model);
    }
}