# Hashnote SDYC Audit
> draft date: Mar 26

# Medium

## [M01] TellerV2 can be drained if `nextPrice` is incorrectly set.

When the Teller V2 contract quotes a price for `buy`, it checks `nextPrice` if it's after trading hours. However, when a user is selling, it always looks at `latestRoundData`.

There is a free arbitrage opportunity after trading hours if `nextPrice` is lower than the current price. This is possible because there are no checks on `nextPrice` when setting them through `reportBalance` in `YieldTokenAggregator`.

### POC

The following script shows how arbitrage can be done when `nextPrice` is lower than lastAnswer.

// contract YieldTokenTellerV2BuySellTest is YieldTokenTellerV2Fixture {
function testArbitrageTellerV2() public {
		// state: address(this) has 1M USDC

    // 1M existing fund in teller
    usdc.mint(address(teller), 1_000_000 * 1e6);

    usdc.approve(address(sdyc), type(uint256).max);
    
    // non-trading hour
    uint256 MAR_25_2024_04_00_ET = 1711353600;

    // admin reported few hours before
    uint256 MAR_24_2024_16_00_ET = MAR_25_2024_04_00_ET - 12 hours;

    oracle.setMockState(2, int256(1.02e8), MAR_24_2024_16_00_ET);
    oracle.setMockTomorrowPrice(int256(1e8));

    
    // after the trading hour, user can buy with 1 and sell with 1.02
    uint usdcBefore = usdc.balanceOf(address(this));
    uint tellerUSDCBefore = usdc.balanceOf(address(teller));

    // buy 1M SDYC with 1M USDC
    teller.buy(usdcBefore);
    uint sdycBalance = sdyc.balanceOf(address(this));
    teller.sell(sdycBalance);

    uint usdcAfter = usdc.balanceOf(address(this));
    uint tellerUSDCAfter = usdc.balanceOf(address(teller));

    uint profit = usdcAfter - usdcBefore;

    // 20K profit for the hacker
    assertEq(profit, 20000 * 1e6);
    assertEq(tellerUSDCBefore - tellerUSDCAfter, 20000 * 1e6);
}

### Mitigation

Check nextPrice is always higher than last answer in `YieldTokenAggregator`, or use nextPrice for selling as well

# Low

## [L01] Teller Accepts Sales at Zero Price

### Impact: Potential Loss of Funds for Users

When a teller contract is deployed with a new oracle, both the price and nextPrice are set to zero. The teller contract doesn't verify if the price is set before a buy or sell operation.

The `buy` function is automatically protected. It takes a stable amount as input and calculates the amount of SDYC to pay out by dividing the stable amount by the price. If the price is zero, the contract reverts, eliminating the risk of buying SDYC at zero price.

However, the `sell` function operates differently. It allows the transaction to proceed under these conditions: it accepts the amount of SDYC to sell and calculates the stable returned to the user by multiplying the SDYC amount by the price. When the price is zero, the contract transfers zero stable to the user, potentially causing a loss of funds for the user.

In this scenario, the contract doesn't lose anything. If it occurs, minters can reimburse the loss to users, so the risk is quite low. However, it's still recommended to perform extra checks. If the teller ever adds features like "buy exact SDYC" in the future, which lets users specify the amount they wish to buy, the absence of this check could allow for the printing of SDYC while paying zero stables.

### POC

contract YieldTokenTellerV2BuySellTestUninitialized is YieldTokenTellerV2Fixture {
    function setUp() public {
        vm.prank(alice);
        usdc.transfer(address(teller), 250_000e6);

        uint256 JUN_5_2023_9_30_00_ET = 1685971800;
        vm.warp(JUN_5_2023_9_30_00_ET);

        rolesAuthority.setUserRole(bob, Role.Investor_MFFeederDomestic, true);
        
        // no oracle initialization!
    }

    function testSellNoPrice() public {
        uint256 amount = 1750 * 1e6;
        vm.prank(address(teller));
        sdyc.mint(alice, amount);

        uint256 usdcBalanceBefore = usdc.balanceOf(address(alice));
        uint256 sdycBalanceBefore = sdyc.balanceOf(address(alice));

        vm.prank(alice);
        teller.sell(amount);

        uint256 usdcBalanceAfter = usdc.balanceOf(address(alice));
        uint256 sdycBalanceAfter = sdyc.balanceOf(address(alice));
        assertEq(usdcBalanceAfter, usdcBalanceBefore);
        assertEq(sdycBalanceAfter, 0);
    }
}

### Mitigation

Add simple `price` check in both buy and sell function.

##

# Informational

## [I01] - `reportBalance` can be used maliciously to cause some side effects, including stealing of shares

The owner of `YieldTokenAggregator` can manipulate the price with `reportBalance`, by setting arbitrary `principal`, `interest`, or `totalSupply` as input.

For example, if the reporter sets `totalSupply` to a very high number, it will push the share price down to a low number, thereby causing the sdyc contract to print a large amount of sdyc as a fee for the feeRecipient.

Since the admin of SDYC already controls `feeRecipient`, `managementFee`, and has the privilege to set minters and their allowance, the admin doesn’t have any motivation to collude with the `YieldTokenAggregator` owner to bypass any setting it already controls.

## [I02] `ShortDurationYieldCoin` only works with oracles with 8 decimals.

In the `_depositFor` and `_withdrawTo` functions, they assume `answer` is always in 8 decimals:

// depositFor
(, int256 answer,,,) = oracle.latestRoundData();
amount = amount.mulDivDown(1e8, uint256(answer));

// withdraw to
(, int256 answer,,,) = oracle.latestRoundData();
amount = _amount.mulDivDown(uint256(answer), 1e8);

However, in the `setOracle` function, there is only a restriction that oracle decimals must be ≥ yield token decimals, which implies the contract is compatible with other oracles with different decimals too.

if (oracle.decimals() < decimals()) revert BadOracleDecimals(); 

### Mitigation

It’d be clearer what the expectation is, by restricting `oracle.decimals()` to be 8.

## [I03] Precision Loss in `processFee`

In `processFee`, the function first calculates the fee in USD terms (interest * management fee percentage), and then converts it to SDYC by dividing the price:

// part 1: take fee as a percentage of interest
uint256 fee = _interest.mulDivDown(mgmtFee, HUNDRED_PCT);

// part 2: converting decimal & price
fee = fee.mulDivDown(1e12, _price);

Part 1 divides the interest by `HUNDRED_PCT`, which is 1e20, erasing the last 20 digits of the product of interest and management fee. But then it multiplies 1e12 back, this means we actually just want to remove 8 digits, therefore, removing 20 digits will result in some precision loss in the calculation.

### POC

// ShortDurationYieldCoin.t.sol
// contract ShortDurationYieldCoinFeeTest is SDYCAggregatorFixture {

function testPrecisionLoss() public {
    // fee is 1.2%
    sdyc.setManagementFee(1.2e18);
    uint price = 1e8;
    uint256 interest_ = 40_099_50; // 40099.5

    vm.prank(address(aggregator));
    sdyc.processFees(interest_, price);

    assertEq(sdyc.balanceOf(alice),  481_194000);  // 40099.5 * 0.012 = 481.194     
}

$forge test --match-test=testPrecisionLoss -vvv

Result:

Error: a == b not satisfied [uint]
    Expected: 481194000
      Actual: 481190000

### Impact

The impact is minor, considering the default decimal of SDYC is 6. However, if this function is used with different configurations, there's a risk of undercharging the fee.

### Mitigation

// use this one-liner
uint256 fee = _interest.mulDivDown(mgmtFee, _price * 1e8);

This change should only result in a slight reduction in gas cost.

## [I04] Precision Loss in Trade Functions like `buy`, `sell`

Similar to the issue above, in the `_depositFor` function, the user's stable amount is first converted to 2 decimals (cents), and then scaled up to 6 decimals. This causes the amount after 2 decimals not to be counted, but still being charged. Since even in the worst case the difference is < 1 cent, the impact is very low.

This issue also exists in the `deposit` and `withdraw` functions of SDYC.

## [I05] Inconsistent Permission Checks on Token Recipients

In the `ShortDurationYieldCoin` contract, multiple different rules are applied to decide if a recipient can hold any token.

In the `mint` function, the code specifically checks if the recipient can call `transfer`.

// checking that recipient has permissions to hold/transfer token
if (!authority.canCall(_to, address(this), this.transfer.selector)) revert NotPermissioned();

But other than the `mint` function, multiple other functions rely on `_checkPermissions(address)` to check whether the token can be moved to a specific address. For example, `transferFrom` and `depositFor`.

function transferFrom(address _from, address _to, uint256 _amount) public virtual override returns (bool) {
    _checkPermissions(_from);
    _checkPermissions(_to);
		...
}

function _depositFor(address _recipient, uint256 _amount) internal returns (uint256 amount) {
        if (_amount == 0) revert BadAmount();
        if (address(underlying) == address(0)) revert NoUnderlying();
}

The `_checkPermissions` function uses `msg.sig` to check permission with the authority contract. When invoked by `transferFrom`, msg.sig will be the selector of `transferFrom`. This means these permission checks are checking if the recipient can call different functions, instead of considering if `to` can later use the normal `transfer` function.

I’d recommend using a separate function to check if the recipient is `whitelisted`, and consistently use that for all functions that need to move the token to a recipient. The `checkPermission` function can still be used, but only to manage access control on `msg.sender`.

function _checkCanReceive(address _address) internal view {
    if (!authority.canCall(_address, address(this), this.transfer.selector)) revert NotPermissioned();
}

## [I05] `ReentryGuardUpgradable` is not initialized in `YieldTokenTellerV2`

The `__ReentrancyGuard_init()` should be invoked in the init function.

## [I06] No event is emitted for initialization in `YieldTokenTellerV2`

In the constructor, the variable `afterHourTrading` is set without the `AfterHourTradingSet` event being emitted. This is similar to `buyFee`, `sellFee`, and `dedicated`. This might caused some off-chain event-based indexer not picking up the initial value.

## [I07] Incorrect field name in the `FeeRecipientSet` event

event FeeRecipientSet(address minter, address newMinter);

It should be 'recipient' instead of 'minter'.

## [I08] Incorrect comment in `YieldTokenTellerV2.transfer`

- * @param _amount is the amount of Yield Token to sell
+ * @param _amount amount of token to transfer

---