Online virtual currency exchange

Question

This is a trade script for an online virtual currency exchange I have been developing. We have a team of 3, and we all work in our own trees. Since we're all young and not very experienced, we would like some feedback and advice prior to hiring penetration testing.

if (isUserLoggedIn()) 
{ 
//get balances
}

function GetMoney($user, $currency)
{
    $user2 = $user;
    $sql   = @mysql_query("SELECT * FROM balances WHERE `User_ID`='$user2' AND `Coin`='$currency'")
    $id    = @mysql_result($sql, 0, "id");
    if ($id < 1) {
        $old = mysql_result($sql, 0, "Amount");
        return $old;
    } else {
        $old = mysql_result($sql, 0, "Amount");
        return $old;
    }
}
$id = mysql_real_escape_string($_GET["market"]);
$result = mysql_query("SELECT * FROM Wallets WHERE `Id`='$id'");
$name = mysql_real_escape_string(mysql_result($result, 0, "Acronymn"));
$fullname = mysql_real_escape_string(mysql_result($result, 0, "Name"));
if($id == 1) {
    header( 'Location: index.php?page=account' ) ;
    die();
}
if($name == NULL) {
    header( 'Location: index.php?page=invalid_market' ) ;
    die();
}
$market_id = mysql_result($result, 0, "Market_Id");
$SQL2 = mysql_query("SELECT * FROM Wallets WHERE `Id`='$market_id'");
$Currency_1a = mysql_result($SQL2, 0, "Acronymn");
$Currency_1 = mysql_result($SQL2, 0, "Id");
//--------------------------------------
if(isset($_POST["price2"]))
{
    if ($_POST["price2"] > 0 && $_POST["Amount2"] > 0) 
    {
        $PricePer = mysql_real_escape_string($_POST["price2"]);
        $Amount = mysql_real_escape_string($_POST["Amount2"]);
        $X = $PricePer * $Amount;
        $Total = file_get_contents("/system/calculatefees.php?P=" . $X);
        $Fees = file_get_contents("/system/calculatefees2.php?P=" . $X);
        $user_id = $loggedInUser->user_id; 
        if(TakeMoney($Total,$user_id,$Currency_1) == true)
        {
            AddMoney($Fees,101,$Currency_1);
            mysql_query("INSERT INTO trades (`To`,`From`,`Amount`,`Value`,`User_ID`,`Type`,`Fee`,`Total`)VALUES ('$name','$Currency_1a','$Amount','$PricePer','$user_id','$name','$Fees','$Total');");
        }
        else
        {
            echo "<p class='notify-red' id='notify'>You cannot afford that!</p>";
        }
    }
    else
    {
        echo "<p class='notify-red' id='notify'>Please fill in all the forms!!</p>";
    }
}
//--------------------------------------
if (isset($_GET["cancel"])) {

    $ids      = mysql_real_escape_string($_GET["cancel"]);
    $tradesql = @mysql_query("SELECT * FROM trades WHERE `Id`='$ids'");
    $from     = @mysql_result($tradesql, 0, "From");
    $owner    = @mysql_result($tradesql, 0, "User_ID");
    $Fee = @mysql_result($tradesql,0,"Fee");
    $Total = @mysql_result($tradesql,0,"Total");
    $sql = mysql_query("SELECT * FROM Wallets WHERE `Acronymn`='$from'");
    $from_id = mysql_result($sql,0,"Id");
    if(TakeMoney($Fee,101,$from_id,true) == true)
    {
        AddMoney($Total,$owner,$from_id);
    }
    else
    {
        echo "<p class='notify-red' id='notify'>You cannot afford that!</p>";
    }
    mysql_query("DELETE FROM trades WHERE `Id`='$ids'");

}
//--------------------------------------
if(isset($_POST["Amount"]))
{
    if ($_POST["price1"] > 0 && $_POST["Amount"] > 0) 
    {
        $PricePer = mysql_real_escape_string($_POST["price1"]);
        $Amount = mysql_real_escape_string($_POST["Amount"]);
        $Fees = file_get_contents("/system/calculatefees2.php?P=" . $Amount);
        $Total = $Fees + $Amount;
        echo $Total;
        $user_id = $loggedInUser->user_id; 
        if(TakeMoney($Total,$user_id,$id) == true)
        {
            AddMoney($Fees,101,$id);
            mysql_query("INSERT INTO trades (`To`,`From`,`Amount`,`Value`,`User_ID`,`Type`,`Fee`,`Total`)VALUES ('$Currency_1a','$name','$Amount','$PricePer','$user_id','$name','$Fees','$Total');");
        }
        else
        {
            echo "<p class='notify-red' id='notify'>You cannot afford that!</p>";
        }
    }
    else
    {
        echo "<p class='notify-red' id='notify'>Please fill in all the forms!!</p>";
    }
}

?>
<link rel="stylesheet" type="text/css" href="../assets/css/trade.css" />
<link href="assets/css/tables.css" type="text/css" rel="stylesheet" />
<center><h3>Trade <?php echo $fullname; ?></h3></center>
<div id="chart">
<script src="../assets/charts/effects.js"></script>
<script src="../assets/charts/Chart.js"></script>
<script src="../assets/charts/excanvas.js"></script>

<meta name = "viewport" content = "initial-scale = 1, user-scalable = no">



        <canvas id="canvas" height="400" width="700" style="background: #333; margin: 0 auto;" ></canvas>
    <script>

        var lineChartData = {
            labels : ["1 Week","24 Hour","8 Hour","4 Hour","2 Hour","1 Hour","Last Trade"],
            datasets : [
                {
                    //Coin A
                    strokeColor : "#fff",
                    pointColor : "#000",
                    pointStrokeColor : "#fff",
                    data : [<?php echo $trade_data; ?>]
                }
            ]

        }

    var myLine = new Chart(document.getElementById("canvas").getContext("2d")).Line(lineChartData);
    </script>
    </body>
</div>
<div id="boxB">
    <div id="boxA">
        <div id="col1">
            <!-- Sell Form-->
            <?php if (isUserLoggedIn()) { ?>
            <div id="sellform" >
                <center><h2>Sell <?php echo $name; ?></h2></center>
                <form action="index.php?page=trade&market=<?php echo $id; ?>" method="POST" autocomplete="off" history="off"> 
                <label>Amount:</label><input type="text" style="width:100px;" name="Amount" onKeyUp="calculateFees1(this)" id="Amount"/>
                <label>Price:</label><input type="text" style="width:100px;" name="price1" onKeyUp="calculateFees1(this)" id="price1" />
                <label>Receive:</label><input type="text" style="width:100px;" id="earn1"/>
                <label>Total:</label><input type="text" style="width:100px;" id="fee1"/>
                <label>Execute:</label><input type="submit" name="Sell" value="Sell"/>
                </form>
            </div>
            <?php } ?>
            <!--Sell Order Book-->
            <div id="sellorders">
            <?php
                include("open_orders_from.php");
            ?>
            </div>
        </div>
        <div id="col2">
            <!--Buy Form-->
            <?php if (isUserLoggedIn()) { ?>
            <div id="buyform">
                <center><h2>Buy <?php echo $name; ?></h2></center>
                <form action="index.php?page=trade&market=<?php echo $id; ?>" method="POST" autocomplete="off" history="off">
                <label>Amount:</label><input type="text" style="width:100px;" onKeyUp="calculateFees2()" name="Amount2" id="Amount2"/>
                <label>Price:</label><input type="text" style="width:100px;" id="price2" onKeyUp="calculateFees2()" onKeyUp="calculateFees2()" name="price2"/>
                <label>Total:</label><input type="text" style="width:100px;" id="fee2"/>
                <label>Execute:</label><input type="submit" name="Buy" id="Buy" value="Buy"/>
                </form>
            </div>
            <?php  } ?>
            <!--Buy Order Book-->
            <div id="buyorders">
            <?php
                include("open_orders_to.php");
            ?>
        </div>
        </div>
    </div>
</div>
<?php if (isUserLoggedIn()) { ?>
<div id="user-orders">
<?php include("open_orders.php"); ?>
</div>
<?php }  ?>

Generation of Market list/Last price in index.php

<ul id="market-list">
        <?php
        $sql = mysql_query("SELECT * FROM Wallets ORDER BY `Acronymn` ASC");

        while ($row = mysql_fetch_assoc($sql)) {
        if($row["Market_Id"] == 0)
        {

        }
        else
        {
            $sql_2 = mysql_query("SELECT * FROM Trade_History WHERE `Market_Id`='". $row["Id"] . "' ORDER BY Timestamp DESC");

            $last_trade = mysql_result($sql_2,0,"Price");
        ?>
        <li><a href='index.php?page=trade&market=<?php echo $row["Id"]; ?>'><?php echo $row["Acronymn"]; ?> &rarr; BTC <?php echo "<font class='price'>".sprintf("%2.8f",$last_trade)."</font>"; ?></a></li>
        <?php
        }
        }
        ?>          
      </ul>

Chat from index.php

<div id="messages"></div>
<?php

//check if user is banned.


//check if user is logged in then display forms.
if (isUserLoggedIn()){ 
        $username = $loggedInUser->display_username;

    //define color of usernames.
    if(!isUserAdmin($id)) {
        $color = "#999999";
        }else{
        $color = "#FF9900";
        }

echo'
<form id="ajaxPOST" history="off" autocomplete="off">
    <input type="hidden" value="'.$color.'" name="color" />
    <input type="hidden" value="'.$username.'" name="username" />
    <input type="text" id="message" maxlength="255" name="message" />
    <input type="submit" id="chat-submit" value="Post Message" />   
</form>';

} else {

echo'
<div id="LoggedOut"></br><b><center>You must be logged in to chat</center></b></div>';


} 

?>
</div>

jQuery code (index.php)

$(document).ready(function() {
    //load messages
    $('#messages').load('chat/ajaxLOAD.php');


    $('#ajaxPOST').submit(function() {
        $.post('chat/ajaxPOST.php', $('#ajaxPOST').serialize(), function(data){
        //clear the message field
        $('#message').val('');
        //reload messages
        $('#messages').delay(1000).load('chat/ajaxLOAD.php').delay(1000).scrollTop($("#messages")[0].scrollHeight);

        });
        return false; 
    });
});

ajaxPOST.php

include 'chat.config.php';

$color = $_POST['color'];
$username = $_POST['username']; 
$message = $_POST['message'];
if ($_POST['message'] == null) 
{

}else{

$db->Query("INSERT INTO messages (color, username, message) VALUES ('".$color."','".$username."','".$message."')");
}

ajaxLOAD.php

    include 'chat.config.php';

$db->query('SELECT * FROM messages ORDER BY (id) ASC');

$data = $db->GET();

foreach($data as $key => $value) {

    echo "<li id='msg_row'><b style='color: ".$value['color'].";'>".$value['username']."</b> :  <i>".$value['message']."</i></li>";

}

Answer 1

There's quite a few things you can fix here. If it is not already in version control, do that first, and then do a commit for every improvement; that way, if anything goes wrong, you can roll back easily. Git is good, but use whatever you know.

Firstly, the business logic and the page layout need to be separated. Many a tutorial on the web mixes the two up, perhaps because many years ago programmers were just too thrilled with the web's possibilities to remember the benefits of modularisation. So, at the very least, move the first half of the code to a separate file, and then require it in your first line.

That becomes your 'logic' file. Whilst you appear to have a few functions in your logic file, the bulk of this is not split into functions at all, and ought to be. There are too many variables sitting in "global space" that need to be local to a function; this will destroy them when each function ends, which is what we want. That said, a few will need to persist in order to be displayed in your HTML layout; potentially they could be the results of functions, although there are several ways to do it.

One step better would be to use a templating engine of some kind, so that your business logic is wrapped up nicely in objects. However, that might be a case of trying to learn too many things at once, so perhaps leave this one for the time being.

In terms of the logic itself, try to avoid the @ error suppression operator, since it will hide problems from you during development. It is better to test intermediate database operations (from connection, then from the query, then from reading results) and just output an error gracefully.

I noticed also that the if clause in GetMoney has two paths that do exactly the same thing. You may wish to check this.

Where you do a financial transaction, this must be in a database transaction to avoid something being being debited without a corresponding credit - you want this so either everything completes or nothing does.

The file_get_contents refers to an absolute pathname rather than something in the current project. It is better to obtain this path from a configuration file, rather than "hardwiring" it into code. Also, this appears to be a PHP file - note that this PHP will not be run to generate data - file_get_contents will treat it just like a text file.

---

For the template part (essentially the second half of your code) you may find using the colon form of statements easier to read. As you've discovered, braces do work here, but <?php if (isUserLoggedIn()): ?> and <?php endif ?> is much clearer.

I notice from your live demo that you do not have a DOCTYPE, which means your document will not be treated as standards-based. Just add <!DOCTYPE html> to the first line (assuming you are targetting HTML5) and this will be fixed.

The tag </br> is not valid markup - I think you meant the self-closing tag <br />.

The link tag <a href='index.php?page=trade&market=6'> is not valid; you need to encode the special characters. Thus, it should be <a href='index.php?page=trade&amp;market=6'>. See the "View Source" in Firefox - items it does not understand are shaded in red.

Avoid the <font> tag completely; use <span> with a style class.