The bug in the safeTransferFrom function of the OwnershipNFTs contract leads to two critical issues:
Denial of Service for Contracts with Correct Implementation: If the recipient (_to address) is a contract that correctly implements the onERC721Received function, the transfer will revert. The root cause is an incorrect conditional check that causes the transfer to fail, even when the contract correctly returns the onERC721Received function selector. This results in a denial of service (DoS) for valid recipient contracts, preventing them from receiving NFTs.
Transfer to Incorrectly Implemented Contracts: Conversely, contracts that do not correctly implement the onERC721Received function or return arbitrary values can pass the flawed validation. This means NFTs can be mistakenly transferred to contracts that are not designed to hold NFTs, potentially leading to a loss of assets or other unforeseen consequences due to incorrect handling of NFTs.
These issues have significant implications for the integrity and reliability of the NFT transfer process, impacting both security and user trust in the system.
Severity
High: The vulnerability directly impacts the correctness of NFT transfers and may result in a loss of assets, disrupt intended functionality, and prevent valid transfers from occurring. The potential loss of user trust and system reliability due to failed transfers further raises the criticality of this issue.
Proof of Concept
Here is a simplified proof of concept (PoC) demonstrating the issue with the OwnershipNFTs contract:
1. Test.t.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.8.16;
import {Test} from "forge-std/Test.sol";
import {OwnershipNFTs} from "./../src/OwnershipNFTs.sol";
import {SeawaterAMM} from "./../src/SeawaterAMM.sol";
import "./../src/IERC721TokenReceiver.sol";
contract testNft is Test {
OwnershipNFTs public ownershipnft;
SeawaterAMM public seawateramm;
MockContract public mockcontract;
function setUp() public {
mockcontract = new MockContract();
seawateramm = new SeawaterAMM(address(this));
ownershipnft = new OwnershipNFTs("","","", seawateramm);
}
function test_safetranser() public {
// This transfer will fail due to the incorrect check in the OwnershipNFTs contract
ownershipnft.safeTransferFrom(address(this), address(mockcontract), 1);
}
}
contract MockContract {
function onERC721Received(address, address, uint256, bytes memory) pure external returns(bytes4) {
// MockContract correctly implements the function selector
return IERC721TokenReceiver.onERC721Received.selector;
}
}
2. SeawaterAMM.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.8.16;
import {ISeawaterAMM} from "./ISeawaterAMM.sol";
contract SeawaterAMM is ISeawaterAMM {
address mockSender;
constructor(address sender) {
mockSender = sender;
}
function positionOwnerD7878480(uint256) view external returns (address) {
return mockSender;
}
function transferPositionEEC7A3CD(uint256 id, address from, address to) external {}
function positionBalance4F32C7DB(address user) external returns (uint256) {}
}
3. ISeawaterAMM.sol
// SPDX-License-Identifier: MIT
pragma solidity 0.8.16;
interface ISeawaterAMM {
function positionOwnerD7878480(uint256 id) external returns (address);
function transferPositionEEC7A3CD(uint256 id, address from, address to) external;
function positionBalance4F32C7DB(address user) external returns (uint256);
}
A mock contract (MockContract) is created to simulate a valid contract capable of receiving NFTs.
The OwnershipNFTs contract is deployed, which is the contract under test, containing the erroneous safeTransferFrom function.
Test Execution:
In the test_safetranser() function, we attempt to transfer an NFT to the mock contract (MockContract) using the safeTransferFrom() function.
Since MockContract correctly implements the onERC721Received function and returns the expected function selector (IERC721TokenReceiver.onERC721Received.selector), the transfer should succeed.
Error Encountered:
Despite the correct implementation, the transfer fails due to the following incorrect validation in the _onTransferReceived function:
require(data != IERC721TokenReceiver.onERC721Received.selector, "bad nft transfer received data");
The check is incorrect because it uses != (not equal), meaning that even when the correct function selector is returned, the transfer is rejected, causing the transaction to revert with the message "bad nft transfer received data".
Impact:
This error results in valid NFT transfers being rejected (DoS), while potentially incorrect transfers (i.e., to contracts that return arbitrary values or incorrect selectors) could succeed, leading to security and functionality risks.
Test Result
This test fails due to incorrect validation in the safeTransferFrom function. Even though the mock contract returns the correct function selector, the transfer reverts, blocking the valid transfer.
[18881] testNft::test_safetranser()
├─ [11555] OwnershipNFTs::safeTransferFrom(testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], MockContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ ├─ [2435] SeawaterAMM::positionOwnerD7878480(1) [staticcall]
│ │ └─ ← [Return] testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]
│ ├─ [418] SeawaterAMM::transferPositionEEC7A3CD(1, testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], MockContract: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f])
│ │ └─ ← [Stop]
│ ├─ [815] MockContract::onERC721Received(testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], testNft: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 1, 0x)
│ │ └─ ← [Return] 0x150b7a02
│ └─ ← [Revert] revert: bad nft transfer received data
└─ ← [Revert] revert: bad nft transfer received data
Ran 1 test suite in 1.40s (1.31ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
This result highlights the Denial of Service (DoS) issue, with the safeTransferFrom function reverting due to faulty logic.
Tools Used
Foundry: A fast, modular testing framework for smart contracts.
Solidity: Used to write the PoC and simulate the NFT transfer process.
VS Code: IDE for writing and testing Solidity contracts.
Recommended Mitigation Steps
To resolve this issue, the validation logic in the _onTransferReceived function should be corrected. Specifically, the condition checking the return value of onERC721Received should be updated to ensure that the function succeeds only when the correct function selector is returned.
Updated Code
function _onTransferReceived(
address _sender,
address _from,
address _to,
uint256 _tokenId
) internal {
// Only call the callback if the receiver is a contract
if (_to.code.length == 0) return;
bytes4 data = IERC721TokenReceiver(_to).onERC721Received(
_sender,
_from,
_tokenId,
"" // Empty data
);
// Correct the validation logic to check for equality
require(
data == IERC721TokenReceiver.onERC721Received.selector,
"bad nft transfer received data"
);
}
Explanation of the Fix:
The previous validation used !=, which caused valid transfers to fail.
The fix ensures that the validation uses ==, so the function only reverts if the return value is not the expected function selector (IERC721TokenReceiver.onERC721Received.selector).
This change ensures that only contracts that correctly implement the onERC721Received function can receive NFTs, preventing denial of service attacks for valid contracts and improper transfers to incorrect ones.
Additional Considerations:
Test Coverage: After implementing this fix, it is essential to update and expand the test cases to ensure transfers succeed when the recipient contract implements the correct function and fail for contracts with incorrect or missing implementations.
Gas Optimization: While the current issue is critical, consider optimizing gas usage in future reviews for further improvements to the contract.
Conclusion
This bug in the safeTransferFrom function is critical as it prevents valid transfers and allows potential transfers to contracts without proper NFT handling. The proposed fix resolves this issue by correcting the validation logic, ensuring that only contracts with the correct implementation can receive NFTs. It is strongly recommended to implement this fix to prevent further denial of service or security vulnerabilities in the system.
Lines of code
https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/sol/OwnershipNFTs.sol#L93
Vulnerability details
Impact
The bug in the
safeTransferFrom
function of theOwnershipNFTs
contract leads to two critical issues:Denial of Service for Contracts with Correct Implementation: If the recipient (
_to
address) is a contract that correctly implements theonERC721Received
function, the transfer will revert. The root cause is an incorrect conditional check that causes the transfer to fail, even when the contract correctly returns theonERC721Received
function selector. This results in a denial of service (DoS) for valid recipient contracts, preventing them from receiving NFTs.Transfer to Incorrectly Implemented Contracts: Conversely, contracts that do not correctly implement the
onERC721Received
function or return arbitrary values can pass the flawed validation. This means NFTs can be mistakenly transferred to contracts that are not designed to hold NFTs, potentially leading to a loss of assets or other unforeseen consequences due to incorrect handling of NFTs.These issues have significant implications for the integrity and reliability of the NFT transfer process, impacting both security and user trust in the system.
Severity
Proof of Concept
Here is a simplified proof of concept (PoC) demonstrating the issue with the
OwnershipNFTs
contract:1.
Test.t.sol
2.
SeawaterAMM.sol
3.
ISeawaterAMM.sol
4.
IERC721TokenReceiver.sol
Detailed Explanation:
Test Setup:
MockContract
) is created to simulate a valid contract capable of receiving NFTs.OwnershipNFTs
contract is deployed, which is the contract under test, containing the erroneoussafeTransferFrom
function.Test Execution:
test_safetranser()
function, we attempt to transfer an NFT to the mock contract (MockContract
) using thesafeTransferFrom()
function.MockContract
correctly implements theonERC721Received
function and returns the expected function selector (IERC721TokenReceiver.onERC721Received.selector
), the transfer should succeed.Error Encountered:
_onTransferReceived
function:!=
(not equal), meaning that even when the correct function selector is returned, the transfer is rejected, causing the transaction to revert with the message "bad nft transfer received data".Impact:
Test Result
This test fails due to incorrect validation in the
safeTransferFrom
function. Even though the mock contract returns the correct function selector, the transfer reverts, blocking the valid transfer.This result highlights the Denial of Service (DoS) issue, with the
safeTransferFrom
function reverting due to faulty logic.Tools Used
Recommended Mitigation Steps
To resolve this issue, the validation logic in the
_onTransferReceived
function should be corrected. Specifically, the condition checking the return value ofonERC721Received
should be updated to ensure that the function succeeds only when the correct function selector is returned.Updated Code
Explanation of the Fix:
!=
, which caused valid transfers to fail.==
, so the function only reverts if the return value is not the expected function selector (IERC721TokenReceiver.onERC721Received.selector
).onERC721Received
function can receive NFTs, preventing denial of service attacks for valid contracts and improper transfers to incorrect ones.Additional Considerations:
Conclusion
This bug in the
safeTransferFrom
function is critical as it prevents valid transfers and allows potential transfers to contracts without proper NFT handling. The proposed fix resolves this issue by correcting the validation logic, ensuring that only contracts with the correct implementation can receive NFTs. It is strongly recommended to implement this fix to prevent further denial of service or security vulnerabilities in the system.Assessed type
DoS