Platform: Code4rena
Start Date: 05/07/2023
Pot Size: $390,000 USDC
Total HM: 136
Participants: 132
Period: about 1 month
Judge: LSDan
Total Solo HM: 56
Id: 261
League: ETH
Rank: 13/132
Findings: 8
Award: $5,019.06
🌟 Selected for report: 1
🚀 Solo Findings: 2
🌟 Selected for report: carrotsmuggler
Also found by: 0xfuje, Vagner, kaden, ladboy233, rvierdiiev
254.4518 USDC - $254.45
The function setMultiSwapper can be called by the owner to change the address of the swapper used in the contract, but it doesn't also approve the rewardToken to the new address, which will make the function that calls compound revert.
The function that calls compound are emergencyWithdraw and _withdraw which will try to swap the rewardToken to wrappedNative, but if the address of the swapper will be changed by the user calling setMultiSwapper because it will need to use another swapper than intended, the swap function will revert all the time, because the new address of the swapper will not have any allowance of the rewardToken making the transaction fail. This means that anytime the owner tries to emergencyWithdraw or a user tries to _withdraw the transaction will revert and funds will be stuck in the contract if something happened and first swapper used in the constructor is not usable anymore.
Manual review
Approve the rewardToken to the new address of the swapper in the setMultiSwapper.
Other
#0 - c4-pre-sort
2023-08-06T14:46:07Z
minhquanym marked the issue as duplicate of #222
#1 - c4-judge
2023-09-19T14:16:57Z
dmvt marked the issue as satisfactory
🌟 Selected for report: Madalad
Also found by: 0xStalin, 0xTheC0der, 0xfuje, Topmark, Vagner, cryptonue, gizzy, peakbolt, rvierdiiev
30.0503 USDC - $30.05
https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L214-L216 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L235-L238
The burst function is used to do batch multiple calls together and it uses a for statement where it will add the value intended to be used of every action in an accumulator which is then checked to be equal to msg.value, but in the case of TOFT_WRAP this get accounted twice which will make every user, that intends to use this function with burst, pay twice the amount of value intended to be used, value which will be lost in the contract.
As you can see the valAccumulator gets updated at the beginning of every for statement with the _action.value passed by the user https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L214-L216, value which is used in every call depending on which _action.id is provided. In the case where _action.id is equal to TOFT_WRAP, the _action.value gets accounted again in the valAccumulator and then it is passed in the call made on ITapiocaOFT https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L236-L238 but since the _action.value was already accounted before this means that the user needs to provide the double amount of _action.value intended to be used in the TOFT_WRAP for the last last require statement to pass https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L714, value which will be stuck in the contract.
Manual review
Don't account for _action.value a second time in the case of TOFT_WRAP since you already account for it at the beginning of every loop of the for statement.
Other
#0 - c4-pre-sort
2023-08-06T02:26:20Z
minhquanym marked the issue as duplicate of #207
#1 - c4-judge
2023-09-21T12:54:54Z
dmvt changed the severity to 2 (Med Risk)
#2 - c4-judge
2023-09-21T13:05:58Z
dmvt marked the issue as satisfactory
🌟 Selected for report: GalloDaSballo
Also found by: 0xfuje, GalloDaSballo, Ruhum, Vagner, jasonxiale, kutugu, mojito_auditor
58.8874 USDC - $58.89
https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L115 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L145
CompoundStrategy.sol uses the function exchangeRateStored in _currentBalance and _withdraw to check the rate that the that the cToken used should be exchanged at, but that function can return stale values if cToken contract used was not interacted with for some time which could cause wrong price assumptions on the protocol.
Similar to how Chainlink oracles needs different checks for stale price, so the protocol would not use wrong prices assumption, cToken oracles should use exchangeRateCurrent instead of exchangeRateStored since it will also accrue for all of the interest and return the most up-to-date values.
Manual review
The recommendation would be to use exchangeRateCurrent instead of exchangeRateStored to return the most recent value, since in the case where parties didn't interact with the specific cToken recently the rate returned would be a stale one. An example of a similar can be seen in the OpenZeppelin audit of BarnBridge https://blog.openzeppelin.com/barnbridge-smart-yield-bonds-audit more specifically [M06].
Oracle
#0 - c4-pre-sort
2023-08-06T12:41:28Z
minhquanym marked the issue as primary issue
#1 - c4-pre-sort
2023-08-06T12:45:03Z
minhquanym marked the issue as duplicate of #1330
#2 - c4-judge
2023-09-26T14:59:28Z
dmvt marked the issue as satisfactory
🌟 Selected for report: 0xfuje
Also found by: Vagner, hassan-truscova, nadin
141.3621 USDC - $141.36
The function mulDiv that is used multiple times in the TapiocaOptionBroker.sol and AirdropBroker.sol to do different calculation, but the contract twAML.sol is compiled with pragma solidity ^0.8.18 as you can see here https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/twAML.sol#L2 which doesn't allow for overflows, and since the function is not unchecked, mulDiv will not behave as intended since it relies implicitly on overflows.
Here is the UniswapV3 version of the function https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/FullMath.sol#L14-L106 and here is the tapioca version https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/twAML.sol#L6-L106 As you can see they are identical, but the tapioca version will not let overflows happens every time this function is used in the code it could revert and break the functionality.
Manual review
Apply the same strategy as UniswapV3 used in their 0.8.0 versions of their libraries by putting the whole function into an unchecked block as you can see here https://github.com/Uniswap/v3-core/blob/6562c52e8f75f0c10f9deaf44861847585fc8129/contracts/libraries/FullMath.sol#L14-L107
Library
#0 - c4-pre-sort
2023-08-05T10:52:20Z
minhquanym marked the issue as duplicate of #138
#1 - c4-judge
2023-09-19T16:36:03Z
dmvt marked the issue as satisfactory
🌟 Selected for report: 0xfuje
Also found by: Vagner, hassan-truscova, nadin
141.3621 USDC - $141.36
FullMath.sol which is used in OracleMath.sol has the _mulDiv function which is copied from UniswapV3 FullMath library, function which require overflow behavior, but that behavior will not be allowed in the Tapioca OracleMath.sol contract, which would make the _getQuoteAtTick revert most of the time.
The contract FullMath.sol uses solidity version >=0.4.0 as can be seen here https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/external/FullMath.sol#L3 which will allow for overflows, but OracleMath.sol is compiled with pragma solidity ^0.8.7 as can be seen here https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/utils/OracleMath.sol#L3, which will also compile FullMath.sol with the version ^0.8.7 since that's how inheritance works in solidity, making the _mulDiv function reverting all the time on overflows.
Manual review
Use uncheck boxes on the _mulDiv function, instead of using >=0.4.0 version of solidity, since with the inheritance that you are doing in OracleMath.sol this will be still compiled at the OracleMath.sol version
Library
#0 - c4-pre-sort
2023-08-05T10:50:39Z
minhquanym marked the issue as duplicate of #138
#1 - c4-judge
2023-09-19T16:36:00Z
dmvt marked the issue as satisfactory
🌟 Selected for report: 0xfuje
Also found by: Vagner, hassan-truscova, nadin
141.3621 USDC - $141.36
https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/libraries/TickMath.sol#L24-L82 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/libraries/FullMath.sol#L14-L106
The function getSqrtRatioAtTick and mulDiv are used in OracleLibrary.sol, contract which is used in UniswapV3Swapper.sol when getOutputAmount or getInputAmount is called, but since getSqrtRatioAtTick and mulDiv requires overflow behavior, the whole functionality would revert most of the time.
Both mulDiv from FullMath.sol
https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/libraries/FullMath.sol#L14-L106
and getSqrtRatioAtTick from TickMath.sol
https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/libraries/TickMath.sol#L24-L82
are identically to UniswapV3 version
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/FullMath.sol#L14-L106
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/TickMath.sol#L23-L54
but those functions require overflow behavior for their functionality, which is not allowed in the Tapioca versions. Because of that those function would revert most of the time.
Manual review
Implement uncheck boxes on those functions similar to how UniswapV3 have in their 0.8.0 versions of their libraries
Library
#0 - c4-pre-sort
2023-08-05T10:52:08Z
minhquanym marked the issue as duplicate of #138
#1 - c4-judge
2023-09-19T16:35:59Z
dmvt marked the issue as satisfactory
🌟 Selected for report: Vagner
1008.3444 USDC - $1,008.34
The function _getRatioAtTick which is used every time _quoteUniswap is called in Seer.sol, is a adapted version of _getSqrtRatioAtTick from UniswapV3 TickMath library, but even if the function is modified to not consider the square root, a overflow behavior is still desired when the ratio is first calculated and since the solidity version of the contract is ^0.8.7 and no unchecked block is used, the function would revert most of the time since it will not allow for overflows.
As you can see the function _getRatioAtTick https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/utils/OracleMath.sol#L40-L98 is very similar from https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/libraries/TickMath.sol#L23-L54 which requires overflow behavior when calculating the ratio, but since the Tapioca version doesn't use unchecked blocks this function would revert most of the time.
Manual review
Consider using uncheck box the same way UniswapV3 used it for all of their libraries for solidity versions greater than 0.8.0.
Library
#0 - c4-pre-sort
2023-08-06T11:45:59Z
minhquanym marked the issue as primary issue
#1 - c4-sponsor
2023-08-20T15:18:23Z
0xRektora marked the issue as sponsor confirmed
#2 - c4-judge
2023-09-30T12:13:50Z
dmvt marked the issue as selected for report