주요 취약점
[H-01] CAN RECOVER GOBBLERS BURNT IN LEGENDARY MINT
unchecked {
uint256 burnedMultipleTotal; // The legendary's emissionMultiple will be 2x the sum of the gobblers burned.
/*//////////////////////////////////////////////////////////////
BATCH BURN LOGIC
//////////////////////////////////////////////////////////////*/
uint256 id; // Storing outside the loop saves ~7 gas per iteration.
for (uint256 i = 0; i < cost; ++i) {
id = gobblerIds[i];
if (id >= FIRST_LEGENDARY_GOBBLER_ID) revert CannotBurnLegendary(id);
require(getGobblerData[id].owner == msg.sender, "WRONG_FROM");
burnedMultipleTotal += getGobblerData[id].emissionMultiple;
emit Transfer(msg.sender, getGobblerData[id].owner = address(0), id);
}
legenderay gobler를 민팅하려면 일반 gobler를 소각시켜야 한다. 위의 burn logic을 보면 address(0)으로 transfer 함으로써 소각시킨다. 그런데 transferFrom 함수를 보면 require문에서 msg.sender == getApproved[id]만 충족시킨다면 gobler를 이동시킬 수 있다. 이렇게 봐서는 딱히 별 문제가 없을 것 같지만 여기서 말도 안 되는 문제가 발생한다.
function transferFrom(
address from,
address to,
uint256 id
) public override {
require(from == getGobblerData[id].owner, "WRONG_FROM");
require(to != address(0), "INVALID_RECIPIENT");
require(
msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
"NOT_AUTHORIZED"
);
delete getApproved[id];
getGobblerData[id].owner = to;
unchecked {
//..
}
emit Transfer(from, to, id);
}문제가 뭐냐? 소각된 gobler를 transferFrom을 통해 다시 예토전생 시킬 수 있다. 이미 gobler의 owner는 approved 상태일테고, transferFrom의 require문들과 getApproved[id]를 delete 하고 to를 새로운 owner로 할당하는 것까지 완벽하다. 처음에 해당 취약점을 듣고 믿기지가 않았다. 뒤통수를 세게 맞은 느낌이 들었다. 흐름을 이해하기 위해 테스트 코드를 살펴보자.
테스트 코드
function testCanReuseSacrificedGobblers() public {
address user = users[0];
// setup legendary mint
uint256 startTime = block.timestamp + 30 days;
vm.warp(startTime);
mintGobblerToAddress(user, gobblers.LEGENDARY_AUCTION_INTERVAL());
uint256 cost = gobblers.legendaryGobblerPrice();
assertEq(cost, 69);
setRandomnessAndReveal(cost, "seed");
for (uint256 curId = 1; curId <= cost; curId++) {
ids.push(curId);
assertEq(gobblers.ownerOf(curId), users[0]);
}
// do token approvals for vulnerability exploit
vm.startPrank(user);
for (uint256 i = 0; i < ids.length; i++) {
gobblers.approve(user, ids[i]);
}
vm.stopPrank();
// mint legendary
vm.prank(user);
uint256 mintedLegendaryId = gobblers.mintLegendaryGobbler(ids);
// confirm user owns legendary
assertEq(gobblers.ownerOf(mintedLegendaryId), user);
// show that contract initially thinks tokens are burnt
for (uint256 i = 0; i < ids.length; i++) {
hevm.expectRevert("NOT_MINTED");
gobblers.ownerOf(ids[i]);
}
// "revive" burnt tokens by transferring from zero address with approval
// which was not reset
vm.startPrank(user);
for (uint256 i = 0; i < ids.length; i++) {
gobblers.transferFrom(address(0), user, ids[i]);
assertEq(gobblers.ownerOf(ids[i]), user);
}
vm.stopPrank();
}정상적으로 민팅하고 나서 다시 부활시키는 모습을 볼 수 있다. 이를 방지하기 위해선 어떻게 해야할까? 이슈를 제기한 warden의 Recommended Mitigation Steps은 다음과 같다.
Ensure token ownership is reset in the for-loop of the
mintLegendaryGobblermethod. Alternatively to reduce the gas cost ofmintLegendaryGobblerby saving on the approval deletion, simply check thefromaddress intransferFrom, revert if it’saddress(0). Note that the latter version would also require changing thegetApprovedview method such that it checks the owner of the token and returns the zero-address if the owner is zero, otherwise thegetApprovedmethod would return the old owner after the underlying Gobbler was sacrificed.
- for-loop에서 ownership을 리셋할 것.
transferFrom의 from 파라미터가address(0)일 때 revert 되도록 할 것.
[M-01] POSSIBLE CENTRALIZATION ISSUE AROUND RANDPROVIDER
function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
// Revert if waiting for seed, so we don't interrupt requests in flight.
if (gobblerRevealsData.waitingForSeed) revert SeedPending();
randProvider = newRandProvider; // Update the randomness provider.
emit RandProviderUpgraded(msg.sender, newRandProvider);
}admin의 중앙화 이슈는 지금까지 medium issue로 판단되어왔다. admin이 잘못된 random provider 주소로 바꾼다면 nft는 리빌되지 않는다. 당연히 이렇게 함으로써 admin이 얻는 경제적 이득은 거의 없다. 하지만 이와 동시에 팀이 multisig를 사용할거라는 보장도 없다. 경제적 이득과는 상관 없이 프로젝트 전체가 잘못될 수 있는 리스크는 제거해야 한다. 따라서 권력이 지나치게 집중되어 있는 이슈는 해결해야하며, 향후 새로운 provider 주소로 수정할 때 거버넌스를 통해 수정하는 것이 바람직하다.
[M-02] THE REVEAL PROCESS COULD BRICK IF RANDPROVIDER STOPS WORKING
//ArtGobblers.sol
function requestRandomSeed() external returns (bytes32) {
uint256 nextRevealTimestamp = gobblerRevealsData.nextRevealTimestamp;
// A new random seed cannot be requested before the next reveal timestamp.
if (block.timestamp < nextRevealTimestamp) revert RequestTooEarly();
// A random seed can only be requested when all gobblers from the previous seed have been revealed.
// This prevents a user from requesting additional randomness in hopes of a more favorable outcome.
if (gobblerRevealsData.toBeRevealed != 0) revert RevealsPending();
unchecked {
// Prevent revealing while we wait for the seed.
gobblerRevealsData.waitingForSeed = true;
//..
}
// Call out to the randomness provider.
return randProvider.requestRandomBytes();
}
requestRandomSeed()를 호출하면 gobblerRevealsData.waitingForSeed = true;가 되는데 이렇게 되면 upgradeRandProvider와 revealGobblers를 호출할 수 없다.
function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
// Revert if waiting for seed, so we don't interrupt requests in flight.
if (gobblerRevealsData.waitingForSeed) revert SeedPending();
randProvider = newRandProvider; // Update the randomness provider.
emit RandProviderUpgraded(msg.sender, newRandProvider);
}
function revealGobblers(uint256 numGobblers) external {
uint256 randomSeed = gobblerRevealsData.randomSeed;
uint256 lastRevealedId = gobblerRevealsData.lastRevealedId;
uint256 totalRemainingToBeRevealed = gobblerRevealsData.toBeRevealed;
// Can't reveal if we're still waiting for a new seed.
if (gobblerRevealsData.waitingForSeed) revert SeedPending();
// Can't reveal more gobblers than are currently remaining to be revealed with the seed.
if (numGobblers > totalRemainingToBeRevealed) revert NotEnoughRemainingToBeRevealed(totalRemainingToBeRevealed);
//..
}
다시 false로 만들기 위해서는 acceptRandomSeed를 호출해야 한다. 그런데 randProvider만 호출할 수 있기 때문에
function acceptRandomSeed(bytes32, uint256 randomness) external {
// The caller must be the randomness provider, revert in the case it's not.
if (msg.sender != address(randProvider)) revert NotRandProvider();
// The unchecked cast to uint64 is equivalent to moduloing the randomness by 2**64.
gobblerRevealsData.randomSeed = uint64(randomness); // 64 bits of randomness is plenty.
gobblerRevealsData.waitingForSeed = false; // We have the seed now, open up reveals.
emit RandomnessFulfilled(randomness);
}- 만약
randProvider가 제대로 동작하지 않거나 문제가 발생했을 때 requestRandomSeed()가 호출된다면
영원히 upgradeRandProvider와 revealGobblers를 호출할 수 없다. 따라서 upgradeRandProvider를 다음과 같이 수정하는 것이 좋다.
function upgradeRandProvider(RandProvider newRandProvider) external onlyOwner {
if (gobblerRevealsData.waitingForSeed) {
gobblerRevealsData.waitingForSeed = false;
gobblerRevealsData.toBeRevealed = 0;
gobblerRevealsData.nextRevealTimestamp = uint64(nextRevealTimestamp - 1 days);
}
randProvider = newRandProvider; // Update the randomness provider.
emit RandProviderUpgraded(msg.sender, newRandProvider);
}[M-03] WRONG BALANCEOF USER AFTER MINTING LEGENDARY GOBBLER
/*//////////////////////////////////////////////////////////////
LEGENDARY MINTING LOGIC
//////////////////////////////////////////////////////////////*/
// The legendary's emissionMultiple is 2x the sum of the multiples of the gobblers burned.
getGobblerData[gobblerId].emissionMultiple = uint32(burnedMultipleTotal * 2);
//..
// We subtract the amount of gobblers burned, and then add 1 to factor in the new legendary.
getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost + 1);
// New start price is the max of LEGENDARY_GOBBLER_INITIAL_START_PRICE and cost * 2.
legendaryGobblerAuctionData.startPrice = uint120(
cost <= LEGENDARY_GOBBLER_INITIAL_START_PRICE / 2 ? LEGENDARY_GOBBLER_INITIAL_START_PRICE : cost * 2
);
legendaryGobblerAuctionData.numSold += 1; // Increment the # of legendaries sold.
// If gobblerIds has 1,000 elements this should cost around ~270,000 gas.
emit LegendaryGobblerMinted(msg.sender, gobblerId, gobblerIds[:cost]);
_mint(msg.sender, gobblerId);function _mint(address to, uint256 id) internal {
// Does not check if the token was already minted or the recipient is address(0)
// because ArtGobblers.sol manages its ids in such a way that it ensures it won't
// double mint and will only mint to safe addresses or msg.sender who cannot be zero.
unchecked {
++getUserData[to].gobblersOwned;
}
getGobblerData[id].owner = to;
emit Transfer(address(0), to, id);
}legendary gobbler를 민팅하는 과정에서 getUserData[msg.sender].gobblersOwned에 1을 더하는 코드가 중복된다. 이미 _mint 함수에 ++getUserData[to].gobblersOwned;가 들어가 있기 때문에 다음과 같이 수정되어야 한다.
getUserData[msg.sender].gobblersOwned = uint32(getUserData[msg.sender].gobblersOwned - cost);
피드백
- [H-01]과 같은 케이스를 자주 겪어보는 것이 중요하다. 리포트를 계속 읽어나가면서 다양한 취약점을 접하고 경험을 쌓자.
- 만약에 연결고리가 끊어지거나 중간에 제대로 동작하는 부분이 발생한다면? -> 해당 케이스를 가정하고 문제되는 부분을 고민해보기.
- 중복되는 카운트가 없는지 확인할 것.
