First Flight #13: Baba Marta
First Flight #13
Beginner FriendlyFoundryGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

MartenitsaToken::updateCountMartenitsaTokensOwner` allows anybody to update the `countMartenitsaTokensOwner` storage variable thus breaking the accountability system.

feder

Summary

The storage variable mapping(address => uint256) public countMartenitsaTokensOwner; is intended to keep track of the number of tokens owned by a given address.

The function updateCountMartenitsaTokensOwner is meant to update the aforementioned storage variable while transferring tokens between addresses. It decreases the token amount held by the sender and increases it for the receiver.

The issue with the current implementation is that it allows anybody to call the function with any address and operation as parameters, breaking the accountability system.

Vulnerability Details

The function MartenitsaToken::updateCountMartenitsaTokensOwner does not impose any restrictions on who can call it, thus breaking the accountability of the countMartenitsaTokensOwner storage variable.

@> anyone can call this function passing any address and operation
function updateCountMartenitsaTokensOwner(address owner, string memory operation) external {
if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("add"))) {
countMartenitsaTokensOwner[owner] += 1;
} else if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("sub"))) {
countMartenitsaTokensOwner[owner] -= 1;
} else {
revert("Wrong operation");
}
}

Proof of concept

  1. Call MartenitsaToken::getCountMartenitsaTokensOwner(address) for a given target address to know the number of NFTs owned by that address.

  2. Call updateCountMartenitsaTokensOwner() passing the target address and the operation add.

  3. Calling MartenitsaToken::getCountMartenitsaTokensOwner(address) for the target address should increase by one.

PoC

Place the following code in MartenitsaToken.t.sol. It manages to increase the number of owned NFTs without actually minting a new one.

function testBreakUpdateCount() public {
uint256 before = martenitsaToken.getCountMartenitsaTokensOwner(chasy);
vm.prank(chasy);
martenitsaToken.updateCountMartenitsaTokensOwner(chasy, "add");
assert(martenitsaToken.getCountMartenitsaTokensOwner(chasy) == before + 1);
}

Impact

  • Incorrect ownership info accounting.

  • The value returned by MartenitsaToken::getCountMartenitsaTokensOwner cannot be trusted.

  • Both MartenitsaMarketplace::buyMartenitsa and MartenitsaMarketplace::makePresent could be exploited by repeatedly calling updateCountMartenitsaTokensOwner(targetAccount, "sub") for a given targetAccount until countMartenitsaTokensOwner[targetAccount] reaches zero. This will prevent the targetAccount from selling or transferring any tokens.

function buyMartenitsa(uint256 tokenId) external payable {
Listing memory listing = tokenIdToListing[tokenId];
require(listing.forSale, "Token is not listed for sale");
require(msg.value >= listing.price, "Insufficient funds");
​
address seller = listing.seller;
address buyer = msg.sender;
uint256 salePrice = listing.price;
​
martenitsaToken.updateCountMartenitsaTokensOwner(buyer, "add");
// <@ the line below will revert if `countMartenitsaTokensOwner[targetAccount]` is set to zero
// <@ prior to calling this function due to overflow.
martenitsaToken.updateCountMartenitsaTokensOwner(seller, "sub");
​
// Clear the listing
delete tokenIdToListing[tokenId];
​
emit MartenitsaSold(tokenId, buyer, salePrice);
​
// Transfer funds to seller
(bool sent, ) = seller.call{value: salePrice}(""); // @-check what if the seller is a contract?
require(sent, "Failed to send Ether");
​
// Transfer the token to the buyer
martenitsaToken.safeTransferFrom(seller, buyer, tokenId); // @-audit does this revert if the transfer isn't successful?
}

Tools Used

Manual review.

Recommended Mitigation

No need to implementcountMartenistaTokensOwnerto keep track of the number of tokens held by each user. Use ERC721 keeps track of it internally and the amount of tokens can be fetch viaERC721::balanceOf

- mapping(address => uint256) public countMartenitsaTokensOwner;
​
- function updateCountMartenitsaTokensOwner(address owner, string memory operation) public {
- if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("add"))) {
- countMartenitsaTokensOwner[owner] += 1;
- } else if (keccak256(abi.encodePacked(operation)) == keccak256(abi.encodePacked("sub"))) {
- countMartenitsaTokensOwner[owner] -= 1;
- } else {
- revert("Wrong operation");
- }
- }
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Missing access control

Support

FAQs

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