Platform: Code4rena
Start Date: 01/07/2021
Pot Size: $100,000 USDC
Total HM: 10
Participants: 7
Period: 7 days
Judge: ghoulsol
Total Solo HM: 4
Id: 17
League: ETH
Rank: 4/7
Findings: 5
Award: $18,939.00
🌟 Selected for report: 3
🚀 Solo Findings: 1
shw
The safetyCheck function of Buoy3Pool checks the two ratios, a/b and a/c, to be healthy but not the ratio b/c. This ratio may be unhealthy, causing assets (USDC, USDT) to be exchanged at a not-so-good price.
Consider the following situation:
lastRatio[1] and lastRatio[2] are both 1000000 (i.e., price of DAI == USDC == USDT).a/b is 998000, and the ratio a/c is 1002000.a/b and a/c are within tolerance (i.e., the difference between itself and its previous value does not exceeds BASIS_POINTS). However, the ratio b/c is not because it differs from the previous value by approximately 2 * BASIS_POINTS.Referenced code: Buoy3Pool.sol#L87-L96
Check the ratio of b/c to ensure it is also healthy.
#0 - kitty-the-kat
2021-07-14T20:39:08Z
#104
#1 - ghoul-sol
2021-07-26T03:36:13Z
Duplicate of #104 so high risk.
shw
The sortVaultsByDelta function of Exposure does not properly initialize the maxIndex and minIndex variables. Consider an edge case where the delta of the three stable coins are all 0. The maxIndex and minIndex variables will be all 0 and vaultIndexes will be [0, 3, 0], which are invalid. The results of the users' deposits could be affected by this bug.
Referenced code: Exposure.sol#L178-L210
Initialize maxIndex and minIndex to 0 and 1 to handle this edge case while being correct in other cases.
#0 - kitty-the-kat
2021-07-14T20:58:45Z
#2
#1 - ghoul-sol
2021-07-26T14:38:12Z
int256 delta = int256( unifiedAssets[i] - unifiedTotalAssets.mul(targetPercents[i]).div(PERCENTAGE_DECIMAL_FACTOR) );
It seems that there's a mathematical possibility that assets will have an ideal proportion between vaults and delta might be 0.
Duplicate of #2 to high risk.
🌟 Selected for report: shw
3923.9983 USDC - $3,924.00
shw
According to Chainlink's documentation, the latestAnswer function is deprecated. This function does not error if no answer has been reached but returns 0, causing an incorrect price fed to the Buoy3Pool.
Referenced code: Buoy3Pool.sol#L207 Buoy3Pool.sol#L214-L216
Referenced documentation: Chainlink - Deprecated API Reference Chainlink - Migration Instructions Chainlink - API Reference
Use the latestRoundData function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete, for example:
(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData(); require(answeredInRound >= roundID, "..."); require(timeStamp != 0, "...");
#0 - kitty-the-kat
2021-07-14T15:06:34Z
disagree with severity (Low risk) Issue would cause deposits and withdrawals to stop, no funds lost
#1 - ghoul-sol
2021-07-26T15:56:46Z
In my opinion halting the protocol deserves medium risk. While no funds are lost, from brand perspective it's a second worst thing. Keeping as medium risk.
shw
The setWithdrawHandler function of Controller does not check that the provided parameter _emergencyHandler is non-zero. However, a similar parameter _withdrawHandler is checked.
Referenced code: Controller.sol#L105-L110
Add require(_emergencyHandler != address(0), "setWithdrawHandler: 0x"); after line 106.
#0 - kitty-the-kat
2021-07-14T20:48:09Z
#5
#1 - ghoul-sol
2021-07-25T21:05:14Z
Duplicate of #5
🌟 Selected for report: shw
1307.9994 USDC - $1,308.00
shw
The withdrawSingleByLiquidity function of LifeGuard3Pool calls buoy.singleStableToUsd to calculate the return USD amount, which internally calls _stableToUsd with the deposit parameter set to true. A more accurate calculation is to set the deposit parameter to false since this action is a withdrawal. A similar issue exists in the function calcProtocolWithdraw of Allocation, where the current strategy's USD is calculated by buoy.singleStableToUsd.
Referenced code: LifeGuard3Pool.sol#L226 Buoy3Pool.sol#L122 Allocation.sol#L142
Consider adding a new boolean parameter, deposit, to the singleStableToUsd function of Buoy3Pool to indicate whether the action is a deposit or not, as that in the stableToUsd and stableToLp functions.
🌟 Selected for report: shw
shw
The eoaOnly function of Controller checks whether the user is whitelisted using tx.origin. Using tx.origin to authenticate users is generally not a good practice since it can be abused by malicious contracts when whitelisted users are interacting with them. Users have to be very careful to avoid being impersonated when interacting with contracts from other protocols, which could unnecessarily burden users.
Referenced code: Controller.sol#L269
Please refer to the following link for more discussion on tx.origin:
Solidity issue - Remove tx.origin
Change tx.origin at line 269 to msg.sender to ensure that the entity calling the Controller is the one allowed.
#0 - kitty-the-kat
2021-07-14T20:33:54Z
#52
#1 - ghoul-sol
2021-07-26T13:26:22Z
This issue touches on different problem so I'll keep it as stand-alone low risk issue.
🌟 Selected for report: a_delamo
Also found by: 0xRajeev, GalloDaSballo, shw
38.4024 USDC - $38.40
shw
Some functions copy the given array-type parameters to local variables first and provide the local variables to other functions as parameters afterward. The array copying operation can be omitted to save gas.
For example, consider the _stableToLp function in Buoy3Pool. A more gas-efficient implementation is to directly provide tokenAmounts to curvePool, as follows:
function _stableToLp(uint256[N_COINS] memory tokenAmounts, bool deposit) internal view returns (uint256) { require(tokenAmounts.length == N_COINS, "deposit: !length"); return curvePool.calc_token_amount(tokenAmounts, deposit); }
Similarly, we can also optimize the following functions to save gas.
Referenced code: Buoy3Pool.sol#L184-L191 Buoy3Pool.sol#L174-L182 Buoy3Pool.sol#L153-L163 Insurance.sol#L160-L169 Insurance.sol#L316-L320
It is shown as above.
#0 - kitty-the-kat
2021-07-14T15:03:41Z
#27
#1 - ghoul-sol
2021-07-26T03:21:25Z
Duplicate of #27