Open code423n4 opened 2 years ago
Let's dissect the finding to prove whether it's valid or not.
First of all, we do have the pre-conditions for re-entrancy:
check effect interaction
pattern)So in any case this is a report for reEntrancy (medium severity)
However, the warden is showing a specific attack vector that, if proven, allows to steal the majority of funds from the basket.
Let's investigate (with a single re-entrant call example):
The require checks at the top pass, as we're rebalancing the basket, we'll make sure to use an additional inputToken (malicious token), that will call settleAuction
again.
This second call (Call B), will execute as normal, extracting the correct amount of value in return for rebalancing, the new ibRatio is 0.9 as shown by the warden POC.
Call B ends by setting ibRatio to 0.9, and hasBonded
to false (which will cause a revert if you try to perform this without re-entrancy)
However, we have already entered and Call A can resume, it now has ibRatio set to 0.9 which allows it to further extract value (as tokensNeeded
decreases as ibRatio decreases)
This can be extended to have further re-entrant calls and can be effectively executed until the basket is hollowed out
This is a Miro Board to highlight the dynamics of the exploit: https://miro.com/app/board/uXjVOZk4gxw=/?invite_link_id=246345621880
Huge props to the warden, brilliant find!
For mitigation:
Handle
cmichel
Vulnerability details
Note that the
Basket
contract approved theAuction
contract with all tokens and thesettleAuction
function allows the auction bonder to transfer all funds out of the basket to themselves. The only limiting factor is the check afterwards that needs to be abided by. It checks if enough tokens are still in the basket after settlement:The bonder can pass in any
inputTokens
, even malicious ones they created. This allows them to re-enter thesettleAuction
multiple times for the same auction.Calling this function at the correct time (such that
bondTimestamp - auctionStart
makesnewRatio < basket.ibRatio()
), the attacker can drain more funds each time, eventually draining the entire basket.POC
Assume that the current
basket.ibRatio
is1e18
(the initial value). The basket publisher callsbasket.publishNewIndex
with some tokens and weights. For simplicity, assume that the pendingtokens
are the same as tokens as before, only the weights are different, i.e., this would just rebalance the portfolio. The function call then starts the auction.The important step to note is that the
tokensNeeded
value insettleAuction
determines how many tokens need to stay in thebasket
. If we can continuously lower this value, we can keep removing tokens from thebasket
until it is empty.The
tokensNeeded
variable is computed asbasketAsERC20.totalSupply() * pendingWeights[i] * newRatio / BASE / BASE
. The only variable that changes in the computation when re-entering the function isnewRatio
(no basket tokens are burned, and the pending weights are never cleared).Thus if we can show that
newRatio
decreases on each re-entrant call, we can move out more and more funds each time.newRatio decreases on each call
After some time, the attacker calls
bondForRebalance
. This determines thebondTimestamp - auctionStart
value insettleAuction
. The attack is possible as soon asnewRatio < basket.ibRatio()
. For example, using the standard parameters the calculation would be:With our initial assumption of
ibRatio = 1e18
and callingbondForRebalance
after 11,000 seconds (~3 hours) we will get our result thatnewRatio
is less than the initialibRatio
:re-enter on settleAuction
The attacker creates a custom token
attackerToken
that re-enters theAuction.settleAuction
function ontransferFrom
with parameters we will specify.They call
settleAuction
withinputTokens = [attackerToken]
to re-enter several times.In the inner-most call where
newRatio = 0.9e18
, they choose theinputTokens
/outputTokens
parameters in a way to pass the initialrequire(IERC20(pendingTokens[i]).balanceOf(address(basket)) >= tokensNeeded);
check - transferring out any other tokens ofbasket
withoutputTokens
.The function will continue to run and call
basket.setNewWeights();
andbasket.updateIBRatio(newRatio);
which will set the new weights (but not clear the pending ones) and set the newbasket.ibRatio
.Execution then jumps to the 2nd inner call after the
IERC20(inputTokens[i]=attackerToken).safeTransferFrom(...)
and has the chance to transfer out tokens again. It will computenewRatio
with the new loweredbasket.ibRatio
of0.9e18
:newRatio = a - b = 2 * 0.9e18 - 1.1e18 = 0.7e18
. Therefore,tokensNeeded
is lowered as well and the attacker was allowed to transfer out more tokens having carefully chosenoutputWeights
.This repeats with
newRatio = 0.3
.The attack is quite complicated and requires carefully precomputing and then setting the parameters, as well as sending back the
bondAmount
tokens to theauction
contract which are then each time transferred back in the function body. But I believe this should work.Impact
The basket funds can be stolen.
Recommended Mitigation Steps
Add re-entrancy checks (for example, OpenZeppelin's "locks") to the
settleAuction
function.