Closed captainmangoC4 closed 6 months ago
Issue created on behalf of judge in order to split into 2 findings
alex-ppg marked the issue as duplicate of #1517
Similarly to #1313, I would award 75% due to the mitigation being ambiguous yet no such option exists. The Warden classifies the issue properly and produces a PoC to exploit it and as such, I am inclined to award this fully.
alex-ppg marked the issue as satisfactory
Lines of code
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104-L120 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196-L254
Vulnerability details
Impact
Multiple re-entrancy issues exist in the codebase, that break core functionality and allow stealing of user funds. In AuctionDemo.sol contract re-entrancy in
cancelBid
andcancelAllBids
allows stealing of user funds. There are multiple attack surfaces, which include a winner of the auction refunding all of his money and receiving NFT for free, and which also include a possiblity of a malicious bidder to refund 2x of his money. Here is the code snippent that transfers NFT to the winner and pays owner money for that NFT:If
highestBidder
is a contract that implements onIERC721Received, in the callback to that contracthighestBidder
can callcancelAllBids
, which will refund him all of his money back, leaving him with a free NFT. It may leave the contract with no funds, but all other calls that transfer money won't revert, since there are no checks on return value of those calls. Impact of that would be that either owner or other bidders won't receive their money back.Another attack surface that exists in AuctionDemo.sol allows a bidder to refund 2x of his bid. In the
else if
block of theclaimAuction
unsuccessful bidders are refunded:If the bidder is a smart contract, it can implement a
receive
function, that will callcancelAllBids
when money are transferred to them. The impact would be that the last bidder in the array would not receive his funds, because someone already claimed them.Cross-contract re-entrancy also exists in MinterContract.sol and NextGenCore.sol. In order for a user to mint an NFT, he has to call MinterContract::mint(), which will call
mint()
on NextGenCore.mint()
function of the core contract calls_mintProccesing
before updating important state that is responsible for keeping track of minted NFT's._mintProccesing
callssafeMint()
which will trigger the callback to onIERC721Received, in which a malicious user can call mint on the MinterContract again. Because state variablestokensMintedPerAddress
and/ortokensMintedAllowlistAddress
were not updated before that call, a user can essentially mint more NFT that he is allowed to according to maxAllowance. This checks in the MinterContract will be bypassed:The impact is that user can mint more than maxAllowance in the allowlistMint and in the public sale.
Proof of Concept
To test re-entrancy in AuctionDemo.sol you need to create this smart contract:
Then you would need to deploy it in
fixtureDeployments.js
with AuctionDemo.sol:And then add those to the
contracts
infixtureDeployments
:Here is the test case demonstrating this issue:
Please add it to nextGenTest.js and run with
npx hardhat test --grep "Re-entrancy"
.In order to test re-entrancy while minting, create this smart contract:
Deploy it:
Add this test to Re-entrancy context:
Tools Used
Manual review
Recommended Mitigation Steps
Add a re-entrancy guard, put non-reentrant modifier on
mint
in MinterContract and oncancelBid
,cancelAllBids
in AuctionDemo.sol. Adhere to the check-effect interaction pattern. When claim happens inclaimAuction
set theauctionInfoData[_tokenid][i].status
to false, so the check incancelBid
will fail. Inmint
function of NextGenCore, call_mintProccesing
after the state is updated.Assessed type
Reentrancy