Trick or Treat
First Flight #27
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

Centralization control risk, unsafe owner updates.

edmpulasky

Summary

Even when the Protocol says Trustworthy Owner, updates in terms of Treat:cost might generate unfairness and lack of transparency after setting aside a treat Pending:Treat, payments on SpookySwap:resolveTrick might not assume for remaining of previously trickedOrTreated TX, such cost update might say the cost is higher asking for more funds of the buyer.

Note: Spooky Surprises should happen at SpookySwap:trickOrTreat by the uncertainty of paying same/half/double price, not by SpookySwap:resolveTrick, unless the Protocol says so, since there's not Information about I considered it as High Severity.

Vulnerability Details

Owners can manipulate the price of an already tricked Treat, changing their cost, affecting SpookySwap:resolveTrick either for cheaper or expensive cost, as well as potential trades between users with revaluations either positively or negatively in contrast to initial user investment.

function resolveTrick(uint256 tokenId) public payable nonReentrant {
require(pendingNFTs[tokenId] == msg.sender, "Not authorized to complete purchase");
​
string memory treatName = tokenIdToTreatName[tokenId];
@> Treat memory treat = treatList[treatName];
@> uint256 requiredCost = treat.cost * 2; // Double price
uint256 amountPaid = pendingNFTsAmountPaid[tokenId];
uint256 totalPaid = amountPaid + msg.value;
​
@> require(totalPaid >= requiredCost, "Insufficient ETH sent to complete purchase");
​
/*
...impl..
*/
}
​

Impact

users might not find an attractive Protocol or so, due to potential cost increasement or decreasement made by Central Authority or Protocol's owner, basically Unfairness.

Tools Used

  • Manual Review

  • Foundry Testing Tool

function testTreatCostManipulation() public {
protocol.addTreat("candy", 1 ether, "");
​
uint256 nextTokenId = protocol.nextTokenId();
uint256 random;
//Predict randomness as 2 so that Pending path get's hit
while (true) {
uint256 timestramp = block.timestamp;
random = uint256(keccak256(abi.encodePacked(timestramp, address(user), nextTokenId, block.prevrandao))) % 1000 + 1;
if (random == 2) {
break;
}
vm.warp(timestramp + 1);
}
​
vm.prank(user);
//Initial Cost 1 ether
protocol.trickOrTreat{ value: 0.1 ether }("candy");
​
//Owner Update
protocol.setTreatCost("candy", 2 ether);
​
//pendingPayment: 2 * cost - pendingNFTsAmountPaid
//pendingPayment: 2 * 1 ether - 0.1 ether = 1.9 ether;
vm.expectRevert(bytes("Insufficient ETH sent to complete purchase"));
vm.prank(user);
protocol.resolveTrick{ value: 1.9 ether }(nextTokenId);
//Now user is supposed to pay much more than it should have (2 * 2 ether) in total
}

Recommendations

It might depend on the protocol's purpose, but i can be changed as follows:

Disallow Protocol:Owner treat manipulation if someone holds a Treat in pending state.

Updates

Appeal created

bube Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

[invalid] Change cost between the call of trickOrTreat and resolveTrick

Only the owner has the rights to change the cost of the treat. Therefore it is assumed that the owner will not change the cost of the pending NFTs. The owner role is trusted.

edmpulasky Submitter
about 1 year ago
bube Lead Judge
about 1 year ago
edmpulasky Submitter
about 1 year ago
bube Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

[invalid] Change cost between the call of trickOrTreat and resolveTrick

Only the owner has the rights to change the cost of the treat. Therefore it is assumed that the owner will not change the cost of the pending NFTs. The owner role is trusted.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.