McMannaman
commented
2 years ago The protocol is having its own ECDSA signing and verifying mechanism.
This is incorrect. For generating and verifying signature a standard solidity ecrecover is used.
The signing is made by trusted backend. It has message constructed with a very specific structure not to be reused or be malleable.
This is too wide of a code reference. The verifying of signature is one execution flow. The MPT proof is completely another. MPT proof is not also made from scratch it is strictly after how Eth signs and forms block hashes.
Overall -- this is not an issue. Particular details should be researched and stated. Should also mention that 'mentor-ish' tone is hardly acceptable.
ak1
high
Implementation of own signing and verifying mechanism is more dangerous.
Summary
The protocol is having its own ECDSA signing and verifying mechanism. This could be more dangerous. The standard EPI 721 protocol is not considered.
Vulnerability Detail
I am sharing the some of the useful resources that could be helful to understand the risk of the issue. https://docs.openzeppelin.com/contracts/2.x/api/cryptography#ECDSA-recover-bytes32-bytes- https://eips.ethereum.org/EIPS/eip-712
All the above links share the information on the standard EIP 712 implementation.
The mover developer its own sign and verify mechanism. For example, it does not consider the domain separator, nonce and how the hashing should be.
https://docs.openzeppelin.com/contracts/2.x/api/cryptography#ECDSA-recover-bytes32-bytes-:~:text=hash%20must%20be,toEthSignedMessageHash%20on%20it.
Impact
This could be a backdoor to many unknow issues.
Code Snippet
https://github.com/sherlock-audit/2022-10-mover/blob/main/cardtopup_contract/contracts/HardenedTopupProxy.sol#L460-L729
Tool used
Manual Review
Recommendation
It is strongly recommended to use the well battle tested EIP 712 based implementation. Kindly refer the openzeppalin implementation. This is majorly used. This will prevent some unknows issue that the mover is missed to implement.