주요 결함들
[H-01] TRANSFERING FUNDS TO YOURSELF INCREASES YOUR BALANCE
function _transfer(
address _from,
address _to,
uint256 _id,
uint256 _amount
) internal virtual {
uint256 _fromBalance = _balances[_id][_from];
if (_fromBalance < _amount) revert LBToken__TransferExceedsBalance(_from, _id, _amount);
_beforeTokenTransfer(_from, _to, _id, _amount);
uint256 _toBalance = _balances[_id][_to];
unchecked {
_balances[_id][_from] = _fromBalance - _amount;
_balances[_id][_to] = _toBalance + _amount;
}
_remove(_from, _id, _fromBalance, _amount);
_add(_to, _id, _toBalance, _amount);
}_transfer 함수는 단순히 from 주소에서 to 주소로 토큰을 이동시키고 있다. 주소에 대한 확인이 없기 때문에 자기 자신에게 토큰을 전송하면 말 그대로 돈복사가 일어나는 결함이 발생한다. 따라서 from != to를 체크하는 것이 가장 좋은 예방책이며 또는 다음과 같이 수정해야 한다.
File: LBToken.sol
189: unchecked {
190: _balances[_id][_from] -= _amount;
191: _balances[_id][_to] += _amount;
192: }[H-02] INCORRECT OUTPUT AMOUNT CALCULATION FOR TRADER JOE V1 POOLS
function _swapSupportingFeeOnTransferTokens(
address[] memory _pairs,
uint256[] memory _pairBinSteps,
IERC20[] memory _tokenPath,
address _to
) private {
IERC20 _token;
uint256 _binStep;
address _recipient;
address _pair;
IERC20 _tokenNext = _tokenPath[0];
unchecked {
for (uint256 i; i < _pairs.length; ++i) {
_pair = _pairs[i];
_binStep = _pairBinSteps[i];
_token = _tokenNext;
_tokenNext = _tokenPath[i + 1];
_recipient = i + 1 == _pairs.length ? _to : _pairs[i + 1];
if (_binStep == 0) {
(uint256 _reserve0, uint256 _reserve1, ) = IJoePair(_pair).getReserves();
if (_token < _tokenNext) {
uint256 _balance = _token.balanceOf(_pair);
uint256 _amountOut = (_reserve1 * (_balance - _reserve0) * 997) / (_balance * 1_000);
IJoePair(_pair).swap(0, _amountOut, _recipient, "");
} else {
uint256 _balance = _token.balanceOf(_pair);
uint256 _amountOut = (_reserve0 * (_balance - _reserve1) * 997) / (_balance * 1_000);
IJoePair(_pair).swap(_amountOut, 0, _recipient, "");
}
} else {
ILBPair(_pair).swap(_tokenNext == ILBPair(_pair).tokenY(), _recipient);
}
}
}
}swap을 진행할 때 _swapSupportingFeeOnTransferTokens 함수를 호출하게 되어있다. v1의 페어일 경우(_binStep == 0) 다음과 같은 공식에 따라 토큰의 양이 결정된다.
uint256 _amountOut = (_reserve1 * (_balance - _reserve0) * 997) / (_balance * 1_000);하지만 원래 v1에서의 계산 방식은 조금 다르다.
function getAmountOut(
uint256 amountIn,
uint256 reserveIn,
uint256 reserveOut
) internal pure returns (uint256 amountOut) {
if (amountIn == 0) revert JoeLibrary__InsufficientAmount();
if (reserveIn == 0 || reserveOut == 0) revert JoeLibrary__InsufficientLiquidity();
uint256 amountInWithFee = amountIn * 997;
uint256 numerator = amountInWithFee * reserveOut;
uint256 denominator = reserveIn * 1000 + amountInWithFee;
amountOut = numerator / denominator;
}JoeLibrary.sol에서는 분모에 amountInWithFee까지 더해서 나누게 되어있다. 따라서 계산 결과가 달라질 수 밖에 없다. 원래 v1 공식에 맞춰서 계산식을 수정해야 한다.
[H-03] WRONG IMPLEMENTATION OF FUNCTION LBPAIR.SETFEEPARAMETER CAN BREAK THE FUNCIONALITY OF LBPAIR AND MAKE USER’S TOKENS LOCKED
struct FeeParameters {
// 144 lowest bits in slot
uint16 binStep;
uint16 baseFactor;
uint16 filterPeriod;
uint16 decayPeriod;
uint16 reductionFactor;
uint24 variableFeeControl;
uint16 protocolShare;
uint24 maxVolatilityAccumulated;
// 112 highest bits in slot
uint24 volatilityAccumulated;
uint24 volatilityReference;
uint24 indexRef;
uint40 time;
}FeeParameters의 첫 8개 field는 144 bits로 구성되어 있고, 나머지 4개의 field는 112 bits로 구성되어 있다.
function _setFeesParameters(bytes32 _packedFeeParameters) internal {
bytes32 _feeStorageSlot;
assembly {
_feeStorageSlot := sload(_feeParameters.slot)
}
uint256 _varParameters = _feeStorageSlot.decode(type(uint112).max, _OFFSET_VARIABLE_FEE_PARAMETERS);
uint256 _newFeeParameters = _packedFeeParameters.decode(type(uint144).max, 0);
assembly {
sstore(_feeParameters.slot, or(_newFeeParameters, _varParameters))
}
}_feeParameters는 FeeParameters 구조로 되어있으며, 해당 슬롯을 가져와 _varParameters와 _newFeeParameters로 decoding 한다.
decode는 2개의 파라미터를 갖는다.
1. 디코딩할 타입의 Maiximum 값
2. 디코딩이 시작되는 offset 위치값
Storage는 다음과 같이 나타낼 수 있다.
Slot(256 bits)
offset 0 144
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
| | |
| 144 bits | 112 bits |
|_ _ _ _ _ _ _ _ _ _|_ _ _ _ _ _|
|| ||
[_newFeeParameters] [_varParameters]
storage에 144 bits가 먼저 들어가서 왼쪽에 위치하고, 그 다음 112 bits가 들어가서 오른쪽에 위치한다. decoding을 할 때에는 112 bits를 먼저 디코딩하고, 이후 나머지 144 bits를 디코딩한다. 처음 112 bits를 디코딩할 때의 위치는 144bits 이후부터 시작되므로 offset 값은 144가 된다.
문제가 되는 부분은 다음이다.
assembly {
sstore(_feeParameters.slot, or(_newFeeParameters, _varParameters))
}이를 이해하기 위해 or 연산자를 알아보자.
uint256 x = 10;
// binary: 0000000000000000000000000000000000000000000000000000000000001010
uint256 y = 7;
// binary: 0000000000000000000000000000000000000000000000000000000000000111
uint256 z = x | y;
// binary: 0000000000000000000000000000000000000000000000000000000000001111or 연산자는 둘 중에 1이 하나라도 있으면 1을 리턴한다. 따라서 z는 15값을 갖는다.
원래 의도대로라면 _newFeeParameters와 _varParameters 변수 중 하나를 _feeParameters.slot에 저장해야 한다. 하지만 단순히 위 코드는 두 변수의 or 값 그 자체를 저장하기 때문에 문제가 발생한다. 이를 방지하기 위해서 _varParameters를 왼쪽으로 144만큼 이동시켜야 한다는데 아직까지 100% 이해하지 못했다..
assembly {
sstore(_feeParameters.slot, or(_newFeeParameters, shl(144, _varParameters)))
}내 생각에는 다음과 같은 프로세스가 아닐까 싶다. 값은 임의대로 넣었다.
//_newFeeParameters
144bits(18bytes)
_ _ _ _ _ _
| |
a02c032932...00000000000000
//_varParameters
112bits(14bytes)
_ _ _ _ _ _
| |
0000000000...0b0533...ff321
현재 decoding을 앞,뒤 나눠서 했는데 이대로 or 연산자를 사용해서 비교를 해버리면 의미가 없어진다. 즉, 제대로 비교를 하려면 현재 뒤 쪽에 값이 있는 _varParameters의 값을 앞으로 144bits만큼 당겨서 동등하게 비교가 가능하도록 해줘야 한다. 어디까지나 내 뇌피셜이라 확실하진 않다. 시간이 지나서 다시 봤을 때 내가 잘못 이해한 부분이 보이면 좋을 것 같다.
[M-01] LBROUTER.REMOVELIQUIDITY RETURNING WRONG VALUES
function removeLiquidity(
IERC20 _tokenX,
IERC20 _tokenY,
uint16 _binStep,
uint256 _amountXMin,
uint256 _amountYMin,
uint256[] memory _ids,
uint256[] memory _amounts,
address _to,
uint256 _deadline
) external override ensure(_deadline) returns (uint256 amountX, uint256 amountY) {
ILBPair _LBPair = _getLBPairInformation(_tokenX, _tokenY, _binStep);
if (_tokenX != _LBPair.tokenX()) {
(_tokenX, _tokenY) = (_tokenY, _tokenX);
(_amountXMin, _amountYMin) = (_amountYMin, _amountXMin);
}
(amountX, amountY) = _removeLiquidity(_LBPair, _amountXMin, _amountYMin, _ids, _amounts, _to);
}if 문에서 _tokenX != _LBPair.tokenX()의 조건을 만족할 경우, 토큰 X와 토큰 Y가 서로 바뀌게 된다. 그런데 if문을 빠져나왔을 때 바뀌는 경우를 고려하지 않는다. 이는 나중에 혼란을 야기할 수 있기 때문에 다음과 같이 수정되어야 한다.
if (_tokenX != _LBPair.tokenX()) {
return (amountY, amountX);
}[M-02] BEFORETOKENTRANSFER CALLED WITH WRONG PARAMETERS IN LBTOKEN._BURN
function _burn(
address _account,
uint256 _id,
uint256 _amount
) internal virtual {
if (_account == address(0)) revert LBToken__BurnFromAddress0();
uint256 _accountBalance = _balances[_id][_account];
if (_accountBalance < _amount) revert LBToken__BurnExceedsBalance(_account, _id, _amount);
_beforeTokenTransfer(address(0), _account, _id, _amount);
unchecked {
_balances[_id][_account] = _accountBalance - _amount;
_totalSupplies[_id] -= _amount;
}
_remove(_account, _id, _accountBalance, _amount);
emit TransferSingle(msg.sender, _account, address(0), _id, _amount);
}여기서 문제가 되는 부분은 _beforeTokenTransfer다. 토큰이 소각되기 전에 해당 hook을 호출한다. 코드를 더 자세히 살펴보자.
/// @notice Hook that is called before any token transfer. This includes minting
/// and burning.
///
/// Calling conditions (for each `id` and `amount` pair):
///
/// - When `from` and `to` are both non-zero, `amount` of ``from``'s tokens
/// of token type `id` will be transferred to `to`.
/// - When `from` is zero, `amount` tokens of token type `id` will be minted
/// for `to`.
/// - when `to` is zero, `amount` of ``from``'s tokens of token type `id`
/// will be burned.
/// - `from` and `to` are never both zero.
/// @param from The address of the owner of the token
/// @param to The address of the recipient of the token
/// @param id The id of the token
/// @param amount The amount of token of type `id`
function _beforeTokenTransfer(
address from,
address to,
uint256 id,
uint256 amount
) internal virtual {}
}from과 to 값에 따라 민팅되거나 소각하는 것에 맞춰서 호출되도록 설계되어 있다. 그런데 다시 _burn 코드로 돌아가서 보면 소각을 할 때 파라미터의 순서가 이상하다.
_beforeTokenTransfer(address(0), _account, _id, _amount);소각을 할 때는 to 자리에 address(0)가 있어야 하는데 반대로 되어 있다. 현재 Trader Joe가 돌아가는데는 문제가 없다. 단지 아무 기능 없는 hook을 호출하는 것 뿐이니까. 그런데 Trader Joe를 fork 해서 수정하거나 그 위에 다른 프로젝트가 돌아간다고 했을 때 문제가 발생할 수 있다. 해당 취약점을 제안한 warden의 예시를 보자.
function _beforeTokenTransfer(
address _from,
address _to,
uint256 _id,
uint256 _amount
) internal override(LBToken) {
if (_from == address(0)) {
// Mint Logic
} else if (_to == address(0)) {
// Burn Logic
}
}다른 프로젝트에서 민팅과 소각을 추적해야 한다고 했을 때, 위와 같이 코드를 구현할 수 있다. 그런데 Trader Joe의 _burn 함수에서는 Hook의 파라미터가 반대로 되어있으므로 원래 의도했던 대로 코드가 동작하지 않을 것이다. 따라서 다음과 같이 수정해야 한다.
_beforeTokenTransfer(_account, address(0), _id, _amount);[M-03] FLASHLOAN FEE COLLECTION MECHANISM CAN BE EASILY MANIPULATED
function flashLoan(
address _to,
uint256 _amountXOut,
uint256 _amountYOut,
bytes calldata _data
) external override nonReentrant {
FeeHelper.FeeParameters memory _fp = _feeParameters;
uint256 _fee = factory.flashLoanFee();
FeeHelper.FeesDistribution memory _feesX = _fp.getFeeAmountDistribution(_getFlashLoanFee(_amountXOut, _fee));
FeeHelper.FeesDistribution memory _feesY = _fp.getFeeAmountDistribution(_getFlashLoanFee(_amountYOut, _fee));
(uint256 _reserveX, uint256 _reserveY, uint256 _id) = _getReservesAndId();
tokenX.safeTransfer(_to, _amountXOut);
tokenY.safeTransfer(_to, _amountYOut);
ILBFlashLoanCallback(_to).LBFlashLoanCallback(
msg.sender,
_amountXOut,
_amountYOut,
_feesX.total,
_feesY.total,
_data
);
_feesX.flashLoanHelper(_pairInformation.feesX, tokenX, _reserveX);
_feesY.flashLoanHelper(_pairInformation.feesY, tokenY, _reserveY);
uint256 _totalSupply = totalSupply(_id);
_bins[_id].accTokenXPerShare += _feesX.getTokenPerShare(_totalSupply);
_bins[_id].accTokenYPerShare += _feesY.getTokenPerShare(_totalSupply);
emit FlashLoan(msg.sender, _to, _amountXOut, _amountYOut, _feesX.total, _feesY.total);
}위 코드에서 알 수 있는 Trader Joe의 flashLoan 특징은 두 가지다.
- 페어의 전체 토큰을 빌릴 수 있다.
- 현재 active한 bin에게만 수수료를 제공한다.
2번으로 인해서 해커는 FlashLoan 수수료의 대부분을 가져갈 수 있다. 예를 들어, 현재 active한 bin의 유동성이 거의 없는 상태일 때(가격이 급격히 변하는 상황이라고 가정) 누군가 해당 페어를 상대로 FlashLoan을 일으켰다고 해보자. 이 때 해커는 sandwich 공격이 가능하다. FlashLoan이 실행되기 전에 active한 bin에 유동성을 제공하고 FlashLoan이 끝나면 곧 바로 유동성을 제거하는 것이다. 만약 bin에 1만큼 유동성이 있는 상태라면 해커는 9만큼 유동성을 제공해서 수수료의 90%를 가져갈 수 있다.
이러한 취약점을 어떻게 알 수 있을까? 만약 FlashLoan 코드에서 수수료를 어떻게 분배하는지 정확히 파악했다면 아래와 같은 사고과정으로 취약점을 찾을 수 있었을 것이다.
수수료가 active한 bin에만 제공된다? -> flashLoan이 발생하기 전에 해당 bin에 유동성을 제공할 수 있지 않을까? -> 대부분의 수수료만 받아오고 바로 유동성을 제거하면 되지 않을까?
취약점을 제안한 warden의 방지책으로는 수수료 체계를 바꾸는 것이다. 토큰 X만 유동성을 제공한 사람은 FlashLoan이 토큰 X에 대해서 발생했을 때만 수수료를 받고 active한 bin이 아닌 모든 bin에 수수료를 제공하도록 변경하는 것. 하지만 토큰 리저브의 변화로 이러한 방안이 실행되기 어렵다면 active한 bin의 앞,뒤로 n 번 째 bin까지 수수료 제공 대상에 포함시키는 것도 대안이 된다.
[M-05] ATTACKER CAN KEEP FEES MAX AT NO COST
Trader Joe는 스왑 수수료에 대해서 위와 같이 계산하고 있다. 전체 계산식은 공식 docs를 확인하자. 위 계산식은 variable fee의 Volatility Accumulator(Va)와 Volatility Reference(Vr)를 어떻게 산출해내는지 설명하고 있다. 이 때 기준이 되는 것은 filter period(tf)로 일정 시간 내에 스왑이 계속 발생하면 Vr을 이전 값과 동일하게 유지하고, decay period(td)를 넘어서면 리셋시킨다. 스왑이 그 사이 시간에 발생하면 Va에 R 값을 곱해준다.
문제는 첫 번째 경우에 발생한다. 변동성이 커져서 Vr이 높은 상태에서 일정 시간 내에 스왑이 계속 발생해 Vr을 이전 값과 동일하게 유지한다면, 수수료는 계속 높은 상태로 유지된다. 이를 이용해 경쟁 프로젝트 측에서 공격을 할 수 있다. 먼저 변동성을 올려놓고 가격에 영향을 주지 않는 미미한 양의 토큰을 스왑한다면 비용을 거의 들이지 않고 수수료를 최대치로 유지시킬 수 있다. 수수료가 매우 높다면 사용자들은 Trader Joe를 사용할 이유가 없고 다른 AMM을 이용할 것이다.
이를 방지하기 위한 예방책은 수수료를 부과하는 체계를 바꾸는 것이다. T < Tf 를 기준으로 했을 때 Td 값을 계속 줄여가는 것이 하나의 방법이 될 것이고, 스왑 결과가 Vr 값에 영향을 주지 않는다면 Tf 값을 업데이트 하지 않는 것이 좋다. 그렇게 해야 t 값만 변동이 생기면서 Vr이 이전 값을 계속 가져오는 것을 막을 수 있다. 마지막으로 스왑의 영향이 미미한 경우에 Vr 값을 업데이트 하지 않도록 해야 한다.
[M-06] CALLING SWAPAVAXFOREXACTTOKENS FUNCTION WHILE SENDING EXCESS AMOUNT CANNOT REFUND SUCH EXCESS AMOUNT
/// @notice Swaps AVAX for exact tokens while performing safety checks
/// @dev will refund any excess sent
/// @param _amountOut The amount of tokens to receive
/// @param _pairBinSteps The bin step of the pairs (0: V1, other values will use V2)
/// @param _tokenPath The swap path using the binSteps following `_pairBinSteps`
/// @param _to The address of the recipient
/// @param _deadline The deadline of the tx
/// @return amountsIn Input amounts for every step of the swap
function swapAVAXForExactTokens(
uint256 _amountOut,
uint256[] memory _pairBinSteps,
IERC20[] memory _tokenPath,
address _to,
uint256 _deadline
)
external
payable
override
ensure(_deadline)
verifyInputs(_pairBinSteps, _tokenPath)
returns (uint256[] memory amountsIn)
{
if (_tokenPath[0] != IERC20(wavax)) revert LBRouter__InvalidTokenPath(address(_tokenPath[0]));
address[] memory _pairs = _getPairs(_pairBinSteps, _tokenPath);
amountsIn = _getAmountsIn(_pairBinSteps, _pairs, _tokenPath, _amountOut);
if (amountsIn[0] > msg.value) revert LBRouter__MaxAmountInExceeded(msg.value, amountsIn[0]);
_wavaxDepositAndTransfer(_pairs[0], amountsIn[0]);
uint256 _amountOutReal = _swapTokensForExactTokens(_pairs, _pairBinSteps, _tokenPath, amountsIn, _to);
if (_amountOutReal < _amountOut) revert LBRouter__InsufficientAmountOut(_amountOut, _amountOutReal);
if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);
}위 함수는 AVAX 토큰을 다른 토큰으로 스왑해준다. 문제가 되는 부분은 가장 마지막 부분이다. 스왑하는 금액의 초과분을 다시 돌려주려고 하고 있다.
if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, amountsIn[0] - msg.value);if문을 통과하려면 msg.value가 더 커야 한다. 그런데 막상 amount를 계산할 때는 더 큰 값을 빼주게 되므로 항상 음수가 되면서 revert가 난다. 결국 함수가 제대로 된 역할을 하지 못하게 된다. 따라서 다음과 같이 수정해야 한다.
if (msg.value > amountsIn[0]) _safeTransferAVAX(_to, msg.value - amountsIn[0]);피드백
- 토큰이 이동할 때 주소에 대한 체크가 빠져있는지 반드시 확인!
- v2 컨트랙트에서 v1 방식의 계산을 지원할 때 계산식이 이전과 동일한지 확인하자.
- 해당 함수가 정확히 어떻게 돌아가는지 파악해야 한다. 수수료를 부과한다면 누구에게 수수료를 주고, 얼만큼의 수수료를 제공하는지까지 알 수 있어야 한다.
- 계산 방식이나 공식은 정확히 이해하도록 하자. 그래야 함수가 어떻게 돌아가는지 알 수 있다.
출처 및 참고자료
