SWC-117 signature and ecrecover

Question

source code is here.

As the SWC-117 says "A signature should never be included into a signed message hash to check if previously messages have been processed by the contract.". I don't know what happened in this contract? How to Hack? What problem is in it?

 pragma solidity ^0.4.24;
 contract transaction_malleablity{
     mapping(address => uint256) balances;
     mapping(bytes32 => bool) signatureUsed;
     constructor(address[] owners, uint[] init){
         require(owners.length == init.length);
         for(uint i=0; i < owners.length; i ++){
         balances[owners[i]] = init[i];
         }
     }
     function transfer(bytes _signature, address _to, uint256 _value, uint256 _gasPrice, uint256 _nonce) public returns (bool){
         bytes32 txid = keccak256(abi.encodePacked(getTransferHash(_to, _value, _gasPrice, _nonce), _signature));
         require(!signatureUsed[txid]);
         address from = recoverTransferPreSigned(_signature, _to, _value, _gasPrice, _nonce);
         require(balances[from] > _value);
         balances[from] -= _value;
         balances[_to] += _value;
         signatureUsed[txid] = true;
     } // something wrong
     function recoverTransferPreSigned(bytes _sig, address _to, uint256 _value, uint256 _gasPrice, uint256 _nonce) public view returns (address recovered) {
         return ecrecoverFromSig(getSignHash(getTransferHash(_to, _value, _gasPrice, _nonce)), _sig);
     }
     function getTransferHash(address _to, uint256 _value, uint256 _gasPrice, uint256 _nonce) public view returns (bytes32 txHash) {
         return keccak256(address(this), bytes4(0x1296830d), _to, _value, _gasPrice, _nonce);
     }
     function getSignHash(bytes32 _hash) public pure returns (bytes32 signHash){
         return keccak256("\x19Ethereum Signed Message:\n32", _hash);
     }
     function ecrecoverFromSig(bytes32 hash, bytes sig) public pure returns (address recoveredAddress) {
         bytes32 r;
         bytes32 s;
         uint8 v;
         if (sig.length != 65) return address(0);
         assembly {
             r := mload(add(sig, 32))
             s := mload(add(sig, 64))
             v := byte(0, mload(add(sig, 96)))
         }
        if (v < 27) {
           v += 27;
         }
         if (v != 27 && v != 28) return address(0);
         return ecrecover(hash, v, r, s);
     }
 }

Answer 1

This is how someone can exploit it:

1. Alice sends some tokens to Bob using the transfer function. All normal so far, since txid wasn't seen before then signatureUsed[txid] == false and the payment goes through.
2. Bob is our exploiter. He picks the signature = (r, s, v) used by Alice and creates a new one signature2 = (r2, s2, v2) like this:

r2 = r
s2 = s
v2 = v<27 ? v+27 : v-27

3. Bob calls transfer with the same parameters used by Alice but using signature2. txid will be different than before so signatureUsed[txid] == false. The signature is also recognized as a valid Alice signature (see how ecrecoverFromSig handles v ...). So the payment goes through.
4. At then end Bob stole an extra payment from Alice.

The root of the problem is that it's possible to use a signature to make another valid one. This opens up to "replay attacks".

This particular code tries to prevent replay attacks by checking if txid was seen before. However txid was calculated using the signature, thus failing to prevent this exploit. The fix is just to remove the signature from txid.