Open sherlock-admin2 opened 3 months ago
Escalate
In OpenZeppelin version 4.9.5, safeIncreaseAllowance
does not call forceApprove
, and during a swap, the actual transfer amount is determined by swapPayload
. This allows users to set the swapPayload
amount lower than params_.swapData.amountInSwap
, ultimately causing issues with USDT support. This behavior is inconsistent with the Sherlock documentation, which states that USDT is supported, thereby violating the main invariant.
Escalate In OpenZeppelin version 4.9.5,
safeIncreaseAllowance
does not callforceApprove
, and during a swap, the actual transfer amount is determined byswapPayload
. This allows users to set theswapPayload
amount lower thanparams_.swapData.amountInSwap
, ultimately causing issues with USDT support. This behavior is inconsistent with the Sherlock documentation, which states that USDT is supported, thereby violating the main invariant.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
This is likely a valid medium, since the swapPayload
can differ from the amountInSwap
To clarify what's the impact here, if someone swaps via Arrakis with params_.swapData.amountInSwap
and params_.swapData.swapPayload
being different, then RouterSwapExecutor
will be unable to make more swaps after it?
Also swap
in RouterSwapExecutor
has onlyRouter
modifier, I assume it's a contract, could you please forward me to the code where the router calls RouterSwapExecutor
.
And in ValantisHOTModule
, there's an onlyManager
modifier, hence, I believe they're trusted to provide correct inputs.
@WangSecurity Hi, the entire project has two different swaps: one in the RouterSwapExecutor contract and the other in the ValantisHOTModule.
The swap in RouterSwapExecutor is called by the related swap function of ArrakisPublicVaultRouter. This allows anyone to call it, so there is a possibility that params_.swapData.amountInSwap
might not equal params_.swapData.swapPayload
.
The swap in ValantisHOTModule is called by the executor from the ArrakisStandardManager contract. Since the executor is not trustworthy during rebalancing, the actual amount passed might not equal the amount used.
When calling the approve function of USDT, it first checks if the approve amount has been set to zero. If not, it will revert.
USDT source code:https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code
Also
swap
inRouterSwapExecutor
hasonlyRouter
modifier, I assume it's a contract, could you please forward me to the code where the router callsRouterSwapExecutor
.
And in ValantisHOTModule, there's an onlyManager modifier, hence, I believe they're trusted to provide correct inputs.
Hi @WangSecurity it may be confusing, but the onlyManager
modifier does not mean that trusted inputs are provided. The ValantisHOTModule
an Arrakis contract, which is meant to talk to a HOT
ALM from Valantis.
So the 'manager' is not a trusted valantis manager, but is actually the ArrakisStandardManager
contract which is the entry-point for the malicious executor to perform rebalances.
And to 100% clarify, the entry-point of this attack is rebalance
function inside ArrakisStandardManager
?
And to 100% clarify, the entry-point of this attack is
rebalance
function insideArrakisStandardManager
?
The entry point for the attack is not only rebalance
; there is also a path through the swap-related functions of the RouterSwapExecutor
contract. Unlike rebalance
, this path can be called by any user.
Therefore, this issue is not the same as the issue with rebalance
as the entry point.
@WangSecurity Hi, I'd like to provide some additional context to explain why this issue is different from the malicious router and malicious payloads category.
RouterSwapExecutor
is an expected behavior.
For example, when calling the swap function of a SovereginPool, the amount spent may not exactly equal params_.swapData.amountInSwap
because the SovereginPool calculates the amount before invoking the transferFrom
. The calculations are done using rounding down, so the actual amount used may be less than params_.swapData.amountInSwap
.
Another example is using Uniswap V3. If the exactOutputSingle
function is used for swapping, it could also result in the consumed amount differing from the allowance. Both of these operations are normal and do not involve any malicious behavior.ArrakisPublicVaultRouter
. Only one of these paths is related to a malicious router. However, considering point two, even if the executor is not malicious, there will still be an issue with USDT not being supported.These three points illustrate that the protocol does not support USDT and also explain why this is not the same type of vulnerability as malicious payloads and routers.
Thank you for that explanation. Planning to accept the escalation and validate with medium severity. Are there any duplicates?
Thank you for that explanation. Planning to accept the escalation and validate with medium severity. Are there any duplicates?
not have dups
Result: Medium Unique
Hi @cu5t0mPeo, instead of upgrading openzeppelin we can use forceApprove right?
Hi @cu5t0mPeo, instead of upgrading openzeppelin we can use forceApprove right?
Hi @Gevarist , I just realized that the best fix for this issue should be to reset the allowance value to zero after calling swap
. If it is not reset to zero, the executor might be able to use the allowance value to transfer funds from the module
contract (from the pool's manager fee).
@cu5t0mPeo except no fund should stay inside the valantis module that can be exploitable by this extra allowance.
The protocol team fixed this issue in the following PRs/commits: https://github.com/ArrakisFinance/arrakis-modular/pull/88
The Lead Senior Watson signed off on the fix.
cu5t0mPe0
medium
USDT is not supported
Summary
When approving USDT, the allowance value needs to be set to 0 first before it can be used correctly. However, the 4.9.5 version of OpenZeppelin does not internally call
forceApprove
.Vulnerability Detail
Users can perform swap operations using
RouterSwapExecutor
, but the actual amount used inparams_.swapData.amountInSwap
andparams_.swapData.swapPayload
can differ. For USDT, this will result in the contract being unable to use the pool again.Additionally, other parts of the protocol are also affected. For example, in
ValantisHOTModule.swap
, setting the router to a specific SovereignPool and passing parameters that cause the actual balance used to be less than amountIn will result in the allowance not being 0. This prevents the module from directly interacting with the SovereignPool again.Impact
The contract may not work properly
Code Snippet
https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/RouterSwapExecutor.sol#L41-L116 https://github.com/sherlock-audit/2024-03-arrakis/blob/64a7dc6ccb5de2824870474a9f35fd3386669e89/arrakis-modular/src/abstracts/ValantisHOTModule.sol#L326-L416
Tool used
Manual Review
Recommendation
It is recommended to upgrade openzeppelin