gildlab / ethgild

Other
2 stars 3 forks source link

protofire audit owner transfer from #192

Open thedavidmeister opened 2 days ago

thedavidmeister commented 2 days ago

The Receipt contract includes an ownerTransferFrom() function that allows the owner to transfer tokens on behalf of another address. This function is not documented and is not implemented in the supposed owner contract, ReceiptVault.sol.

thedavidmeister commented 1 day ago

ReceiptVault is an abstract contract, so it is not expected to implement all functionality, it's merely a baseline for concrete implementations to extend for a specific use case

OffchainAssetReceiptVault is an implementation of ReceiptVault intended for RWA that need to meet regulatory compliance and so may need to appoint a confiscator to comply with sanctions etc.

its documented where it is used https://github.com/gildlab/ethgild/blob/main/src/concrete/vault/OffchainAssetReceiptVault.sol#L713

that implementation is out of scope for this audit, only the ERC20PriceOracleReceiptVault contract is in scope at this time, for Cyclo, and that contract uses onchain assets only and so is permissionless and has no use case for the receipt vault owner to be moving the receipts unilaterally

For context, the original implementation of the vault was a single contract that was both ERC20 and ERC1155 but it had terrible wallet support, because even MetaMask assumes that a contract only implements a single token interface. For that reason we split the functionality into two, the vault for the ERC20 component, according to ERC4626 and the receipt which is the ERC1155 component. So the ownership pattern of the vault owning the receipt is basically replacing the fact that some of these functions were internal function calls in the original implementation, but now need to be external and so access restricted to the vault controlling the receipt.