Y2k Finance contest - zzzitron's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L67-L82

Vulnerability details

Impact

The function PegOracle:latestRoundData will always return zero for the nowPrice if the priceFeed1.decimals are 18. Even if the priceFeed1.decimals are less than 18 it still returns a wrong value for the nowPrice.

Proof of Concept

In the below code snippet, if the priceFeed1.decimals is 18, then the returned nowPrice will be 0; if it is smaller than 18 it returns a wrong value for the nowPrice.

// PegOracle
// latestRoundData
67        if (price1 > price2) {
68            nowPrice = (price2 * 10000) / price1;
69        } else {
70            nowPrice = (price1 * 10000) / price2;
71        }
72
73        int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
74        nowPrice = nowPrice * decimals10;
75
76        return (
77            roundID1,
78            nowPrice / 1000000,
79            startedAt1,
80            timeStamp1,
81            answeredInRound1
82        );

Tools Used

None

Recommended Mitigation Steps

Use a fixed precision (decimals) for the price.

Example:

int256 precision = 10000;

if (price1 > price2) {
            nowPrice = (price2 * precision) / price1;
        } else {
            nowPrice = (price1 * precision) / price2;
        }

        int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
        nowPrice = nowPrice * decimals10;

        return (
            roundID1,
            nowPrice,
            startedAt1,
            timeStamp1,
            answeredInRound1
        );

<!-- zzzitron H02 -->

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L167

Vulnerability details

Impact

The wrong amount can be transferred from the msg.sender to the Vault.

Proof of Concept

When Vault:deposit is called, it is supposed to get assets amount of asset from the msg.sender and send the msg.sender shares amount of the vault token.

However in the current implementation, the Vault would get shares amount of asset from the msg.sender. See line 167 from the snippet below.

The assets and shares are not necessarily the same amount. Therefore the Vault might transfer the wrong amount of asset from the msg.sender.

// Vault
// deposit
165        require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");
166
167        asset.transferFrom(msg.sender, address(this), shares);
168
169        _mint(receiver, id, shares, EMPTY);

Tools Used

None

Recommended Mitigation Steps

// Vault
// deposit
167        asset.transferFrom(msg.sender, address(this), assets);

<!-- zzzitron H04 -->