ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Check-Effects-Interactions pattern is not followed

paolinapetkova

Summary

In contract Enscrow.sol the functions _depositIntoEscrow() and _withdrawFromEscrow() are not following the Check-Effects-Interactions pattern. I am submitting this as an informational.

Vulnerability Details

In the functions _depositIntoEscrow() and _withdrawFromEscrow() the state is updated after the external call which is not recommended due to security reasons. To follow the CEI first need to update the state and then the external call.

Impact

Not following Solidity best practises may cause additional security issues in future code changes.

Tools Used

Slither, manual review

Recommendations

function _depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
internal
{
assert(ids.length > 0);
​
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
​
+ _escrow[collection][id] = msg.sender;
if (collectionType == CollectionType.ERC721) {
IERC721(collection).transferFrom(msg.sender, address(this), id);
} else {
// TODO: check the supply is exactly one.
// (this is the low level call to verify if a contract has some function).
// (but it's better to check with supported interfaces? It's 2 calls instead
// of one where we control the fail.)
//(bool success, bytes memory data) = contractAddress.call("");
IERC1155(collection).safeTransferFrom(msg.sender, address(this), id, 1, "");
}
​
- _escrow[collection][id] = msg.sender;
}
}
function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
​
address from = address(this);
​
+ _escrow[collection][id] = address(0x0);
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
​
- _escrow[collection][id] = address(0x0);
​
return true;
}
Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Support

FAQs

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