`delegatedFromEthAddress` and `ethAddressFromDelegated` are dangerous

[Stebalien]

There isn't a 1:1 mapping between f4 addresses and 0x addresses.

1. There are multiple (potential) f4 address ranges, FEVM uses f410f.
2. f410f... addresses may not necessarily map to valid 0x addresses. E.g., the payload may be too long, too short, or may be part of the "masked ID address" range.
3. 0xff000000...{8 byte ID} maps to f0, not f410f.

Instead of providing delegatedFromEthAddress and ethAddressFromDelegated, this library should provide filAddressFromEthAddress and ethAddressFromFilAddress.

[navFooh]

Hi @Stebalien, I agree the naming of the methods could improve, but I'm not seeing any major safety concerns. The methods will throw an error if you're trying to perform a conversion that would be considered dangerous.

The only exception is that ethAddressFromDelegated does not currently check if the delegated address has the right namespace (10). Which I'll patch, but since there are currently no other delegated address namespaces, it shouldn't have caused any harm.

Concerning point 2, I assume that any f410f... address on the network should have originally been created from a valid ethereum address and can also be converted back?

See an overview of the available methods and their safety procedures below. If this package can still preform any conversions that would be deemed dangerous or invalid, please provide some examples and I'll make sure to look into them.

// Converts 0x... to f410f...
// Throws an error if the passed ethAddress is not 0x... or if it is 0xff000000...{8 byte ID}
function delegatedFromEthAddress(ethAddr: EthAddress): string

// Converts f410f... to 0x...
// Will update to throw an error if the passed delegated address is not f410f...
function ethAddressFromDelegated(delegated: string): EthAddress

// Returns true if the passed address is 0x...
function isEthAddress(address: string): address is EthAddress

// Returns true if the passed ethAddr is 0xff000000...{8 byte ID}
function isEthIdMaskAddress(ethAddr: EthAddress): boolean

// Converts 0xff000000...{8 byte ID} to f0...
// Will throw an error if the passed ethAddr is not 0xff000000...{8 byte ID}
function idFromEthAddress(ethAddr: EthAddress): string

// Converts f0... to 0xff000000...{8 byte ID}
// Will throw an error if the passed idAddress is not f0...
function ethAddressFromID(idAddress: string): EthAddress

[Stebalien]

You're absolutely right right about the first point, I missed the isEthIdMaskAddress part.

I think users may be calling newDelegatedEthAddress? It looks like that should call delegatedFromEthAddress.

I assume that any f410f... address on the network should have originally been created from a valid ethereum address and can also be converted back?

Not necessarily, unfortunately. Users may also try to decode f4 address from other sources (e.g., the user uses some other tool that messes up when encoding an f4 address).

For the second, part ethAddressFromDelegated, there are actually a few issues:

1. As you noted, it needs a namespace check. I think it also needs an address class (is it f4...), but I didn't check carefully.
2. The length needs to be checked. It can be anywhere between 0 and 54 on the network and users can construct arbitrary addresses.
3. Masked addresses are not rejected. I.e., if a user does end up encoding 0xff.... into an f410f address, ethAddressFromDelegated should fail when trying to convert it back to 0x.

---

Everything else makes sense although I haven't audited the rest of the functions in the library.

[navFooh]

I think users may be calling newDelegatedEthAddress? It looks like that should call delegatedFromEthAddress.

Indeed you're right that newDelegatedEthAddress does not have the ID mask check, good catch. I'll add the guard rails there instead of at delegatedFromEthAddress, so then it will work for both.

Not necessarily, unfortunately. Users may also try to decode f4 address from other sources (e.g., the user uses some other tool that messes up when encoding an f4 address).

Right.. yeah it's tricky with user input. We don't anticipate users entering faulty addresses from other sources, but indeed we can add some checks to prevent a few cases.

1. I've added the namespace check. The f4... check is already there, as it will throw an error when reading the subaddress from a non-f4... adrress, see: https://github.com/glifio/modules/blob/bda440822a03464384f21c2c878edd277bdfd545/packages/filecoin-address/src/index.ts#L105
2. The following line throws if the length of the resulting ethereum address is not valid: https://github.com/glifio/modules/blob/bda440822a03464384f21c2c878edd277bdfd545/packages/filecoin-address/src/index.ts#L401
3. I've added a check to the resulting Eth address to make sure it doesn't return ID masks

I've updated the tests so that the package build will fail if any of these cases are not working properly

[Stebalien]

as it will throw an error when reading the subaddress from a non-f4... adrress, see:

Ah.... implicit properties...

I've updated the tests so that the package build will fail if any of these cases are not working properly

:heart: