Code4rena에서 열리는 대회에 참여하면 보통 1달 정도 뒤에 결과가 나온다. 오늘은 그 동안 여러 대회에 참여하면서 얻은 결과를 정리해보려 한다. 결론부터 말하면 High 1개, Medium 3개 총합 4개의 취약점을 발견했다. 지난 6개월간 노력해서 얻은 성과라고 생각하니 내심 뿌듯하다. 다시 한 번 내가 발견한 취약점을 정리하고 이후 리포트와 비교하면 부족했던 부분을 알 수 있을 것이다.
1. Escher
function getPrice() public view returns (uint256) {
Sale memory temp = sale;
(uint256 start, uint256 end) = (temp.startTime, temp.endTime);
if (block.timestamp < start) return type(uint256).max;
if (temp.currentId == temp.finalId) return temp.finalPrice;
uint256 timeElapsed = end > block.timestamp ? block.timestamp - start : end - start;
return temp.startPrice - (temp.dropPerSecond * timeElapsed);
}더치옥션으로 진행되는 LPDA 세일은 시간이 지날수록 가격이 하락하게 되어있다. 문제가 되는 부분은 getPrice 함수의 마지막 계산 방식이다. 만약 temp.dropPerSecond * timeElapsed이 temp.startPrice 보다 높다면 해당 함수는 revert가 된다. 즉, 옥션 시작 가격보다 더 큰 금액이 차감될 경우 현재 가격을 리턴할 수 없다. getPrice 함수는 buy 함수에도 사용되기 때문에 유저는 결국 구매할 수 없게되고 서비스가 마비된다.
if (temp.startPrice < (temp.dropPerSecond * timeElapsed)) return temp.finalPrice;나는 코드 중간에 위와 같은 if문을 추가해서 바로 최종가격을 리턴하도록 Recommendation을 작성했었다.
require((sale.endTime - sale.startTime) * sale.dropPerSecond <= sale.startPrice, "MAX DROP IS GREATER THAN START PRICE");반면 리포트에서는 require문을 사용해 underflow를 방지하는 코드를 추천했다. 내가 작성한 해결방안 보다 훨씬 깔끔하다고 생각한다. if문 보다는 require문을 활용해 처음부터 underflow 되는 여지를 없애는 것이 더 나은 것 같다.
2. Biconomy
function execute(address dest, uint value, bytes calldata func) external onlyOwner{
_requireFromEntryPointOrOwner();
_call(dest, value, func);
}
function _requireFromEntryPointOrOwner() internal view {
require(msg.sender == address(entryPoint()) || msg.sender == owner, "account: not Owner or EntryPoint");
}execute 는 _requireFromEntryPointOrOwner()로 인해 entryPoint 또는 owner가 호출하도록 되어있다. 하지만 onlyOwner modifier를 사용하기 때문에 결국 entryPoint는 해당 함수를 호출할 수 없다. 따라서 onlyOwner modifier를 제거해야 한다.
나는 execute 함수만 수정해야 한다고 작성했었다. 리포트에서는 execute 뿐만 아니라 executeBatch에서도 같은 이유로 onlyOwner modifier를 제거해야 한다고 나와있다. 취약점을 발견했을 때 다른 함수에도 적용 가능한지 살펴보도록 하자.
3. Caviar
function buyQuote(uint256 outputAmount) public view returns (uint256) {
return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
}문제가 되는 부분은 buyQuote의 계산식이다. 솔리디티에서 나눗셈이 나오면 가장 먼저 Rounding error가 발생할 수 있는지 확인해야 한다. 위 식은 baseTokenReserves가 상대적으로 소량일 때 Rounding error가 발생한다.
function buyQuote(uint256 outputAmount) public view returns (uint256 inputAmount) {
uint256 inputAmount = (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
require(inputAmount > 0, "InputAmount is zero");
return inputAmount;
}이를 방지하기 위해 나는 require문을 사용하는 것을 권장한다고 작성했었다. 하지만 리포트를 보면 내 생각이 잘못 됐을 수도 있다는 생각이 든다.
function buyQuote(uint256 outputAmount) public view returns (uint256) {
return mulDivUp(outputAmount * 1000, baseTokenReserves(), (fractionalTokenReserves() - outputAmount) * 997);
}리포트에서는 mulDivUp을 통해 올림 처리 하는 것을 권장한다. 내가 생각했던 방식을 사용하면 나눗셈 결과가 0이 되는 것을 방지할 수 있겠지만 해당 결과값을 가지고 이후 단계를 진행할 수 없다. 리포트 방식대로 한다면 Rounding error가 되는 결과값들을 올림 처리해서 buyQuote가 값을 리턴할 수 있게된다.
Rounding error를 방지한다는 목적은 같지만 권장하는 방식에서 차이가 났다. 취약점을 제출할 때는 목적에만 집중해서 이후 단계를 진행할 수 있는지에 대해 크게 신경쓰지 않았다. 같은 목적이더라도 이를 해결하는 방식에 따라 결과가 달라질 수 있다는 점을 기억하자.
4. ParaSpace
/// @notice Allows updater to set new price on PriceInformation and updates the
/// internal Median cumulativePrice.
/// @param _asset The nft contract to set a floor price for
/// @param _twap The last floor twap
function setPrice(address _asset, uint256 _twap)
public
onlyRole(UPDATER_ROLE)
onlyWhenAssetExisted(_asset)
whenNotPaused(_asset)
{
bool dataValidity = false;
if (hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
_finalizePrice(_asset, _twap);
return;
}
dataValidity = _checkValidity(_asset, _twap);
require(dataValidity, "NFTOracle: invalid price data");
// add price to raw feeder storage
_addRawValue(_asset, _twap);
uint256 medianPrice;
// set twap price only when median value is valid
(dataValidity, medianPrice) = _combine(_asset, _twap);
if (dataValidity) {
_finalizePrice(_asset, medianPrice);
}
} function _checkValidity(address _asset, uint256 _twap)
internal
view
returns (bool)
{
require(_twap > 0, "NFTOracle: price should be more than 0");
PriceInformation memory assetPriceMapEntry = assetPriceMap[_asset];
uint256 _priorTwap = assetPriceMapEntry.twap;
uint256 _updatedAt = assetPriceMapEntry.updatedAt;
uint256 priceDeviation;
//first price is always valid
if (_priorTwap == 0 || _updatedAt == 0) {
return true;
}
//..
}NFT의 새 가격을 설정할 때 setPrice 함수를 사용한다. 이 때 함수를 호출할 수 있는 조건은 3가지다.
- Updater role을 갖고 있는 계정
- Asset이 존재해야 함
- Asset이 정지되지 않아야 함
따라서 정상적인 상황은 다음과 같다. Updater role을 갖고 있는 계정이 setPrice 함수를 호출해서 이미 존재하는 Asset의 가격을 설정한다. 그런데 만약 setPrice 함수가 front run 공격을 받는다면 어떻게 될까? 누군가가 가격을 조작해서 낮은 가격에 모든 Asset을 탈취해갈 여지가 있다.
굳이 front run 공격이 아니더라도 Updater role 계정이 탈취될 수도 있다. 또한 함수 내 다른 조건들(_checkValidity 등) 같은 경우, 첫 번째 가격은 항상 valid 하다고 판정되기 때문에 쉽게 통과할 수 있다.
//we need to deploy 3 oracles at least
uint8 constant MIN_ORACLES_NUM = 3;Oracle은 최소 3개로 설정되는데, 한 Oracle feeder가 마음만 먹으면 front run을 통해 처음 설정되는 NFT의 가격을 조작할 수 있다.
