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

In `StarklaneEscrow::_isEscrowed` no checks present to ensure address is `msg.sender` or not, can lead to breaking functionality of `Starklane::withdrawTokens` function

tdey

Summary

_isEscrowed function just performs a zero address check, doesn't make sure that the address is actually the msg.sender.
The Starklane::withdrawTokens function calls StarklaneEscrow::_withdrawFromEscrow which calls StarklaneEscrow::_isEscrowed to check if the address is present in the escrow or not. But it doesn't account if the address is actually of the right owner or not.

Vulnerability Details

withdrawTokens function

_withdrawFromEscrow function

_isEscrowed function

depositTokens function

In withdrawTokens function, this line:

bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);

The above line will always return true , because this calls _withdrawFromEscrow function checks if the address is escrowed or not with _isEscrowed function.

_isEscrowed function just checks if the address entered is greater than 0 or not, any address can be passed other than 0 address and it will return true. There is no actual checks present to ensure the address entered is indeed a legitimate escrowed address.

Hence bool wasEscrowed will always be true.

if (!wasEscrowed) {
// TODO: perhaps, implement the same interface for ERC721 and ERC1155
// As we only want to deal with ERC1155 token with value = 1.
// Also, check what to do with URIs. If the URI storage is supported
// or not for ERC721. If supported, we may need to mint with an URI.
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id); //@note basically mints tokens
}

This part from withdrawTokens function will never be executed. And minting will never happen in case the tokens are not present in the escrow.

No checks present in withdrawTokens function to ensure that the tokens can only be withdrawn by the right owner.
Anyone who has deposited once using Starklane::depositTokens function will have their id in escrow as Starklane::depositTokens function updates the id to corresponding msg.sender here :

_escrow[collection][id] = msg.sender;

Hence they can pass the check performed by _isEscrowed easily as their id is already present in escrow. Anyone should be able to take out tokens from escrow without being the owner.

Impact

In case the tokens were not present in the escrow it will not be minted as wasEscrowed will always be true.
Anyone can withdraw anyone else's tokens.

Tools Used

Manual review

Recommendations

In my opinion, this check should be performed along with the one which is already being done in _isEscrowed function which would ensure that only the rightful owner can withdraw funds.

function _isEscrowed(
//@audit it is just checking if the the token address is greater than 0 address,
//that doesn't mean it's the right address?
address collection,
uint256 id
)
internal
view
returns (bool)
{
-- return _escrow[collection][id] > address(0x0);
++ if(_escrow[collection][id] == msg.sender && _escrow[collection][id] > address(0x0)){
++ return true;
++ }
++ else{
++ return false;
++ }
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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