auction.sol

contract Auction {
	
	address payable public beneficiary;
	uint public auctionEndTime;
	
	address public highestBidder;
	uint public highestbid;
	
	event highestBidIncreased(address bidder, uint amount);
	event auctionEnded(address winner, uint amount);
	
	constructor(uint _biddingTime, address payable _beneficiary) {
		beneficiary = _beneficiary;
		auctionEndTime = block.timestamp+ amount;
		
		// Critical error ???: amount variable doesn't exist - should be _biddingTime.
		//This line sets when the auction ends by adding the bidding time to the current time.
		
}

	function bid() public payable {
	
		if(block.timestamp > auctionEndTime) revert('the auction has ended');
		
		if(msg.value <= highestbid) revert('sorry, the bid is not high enough');
		
		if(highestbid != 0) {
			pendingReturns[highestBidder] +=highestbid;
			
			/// pendingReturns mapping is never declared - this is a critical missing variable.
			// This line should record that the previous highest bidder needs their money back.
		
		}
		
		highestBidder = msg.sender;
		higestbid = msg.value;
		emit highestBidIncreased();
		
		
		//Missing parameters - the event expects (address bidder, uint amount)
		//but none are provided. Should be emit highestBidIncreased(msg.sender, msg.value);

	}
	// withdraw bids that were overbid
	
	//Should NOT be payable - this function doesn't need to receive ether, 
	//it sends ether out. This function lets people withdraw their outbid money.
	
	
	function withdraw () public payable returns(bool) {
		uint amount = pendingReturns[msg.sender];
		if(amount > 0) {
			pendingReturns[msg.sender] = 0;
			
		}
		//Dangerous pattern - setting the balance to zero BEFORE sending the money leaves a reentrancy
	//		vulnerability if the send fails. However, this pattern is actually safer than the alternative for preventing reentrancy attacks,
	//but the logic below contradicts this approach. (I dont understand this) 
	//	
	
		if(!payable(msg.sender).send(amount)) {
			pendingReturns[msg.sender] = amount;
			
		}
		return true;
		
	}
	
	//Attempts to send the money. If it fails, it restores the pending amount. 
	//This should also return false when the send fails to properly indicate failure.
	
		
		function auctionEnd() public {
	
			if(block.timestamp > auctionEndTime)revert('the auction has not ended')
			if(ended revert)'the auction is already over!')

//Severe syntax error - should be if(ended) revert('the auction is already over!');. 
//Also, the ended boolean variable is never declared at the top of the contract.


			ended = true;
			emit auctionEnded(highestBidder,highestbid);
			beneficiary.transfer(highestbid);
			
		}
	
	}